Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use annotate-snippets for emitting errors #3507

Merged
merged 13 commits into from
Apr 17, 2019
Merged

Use annotate-snippets for emitting errors #3507

merged 13 commits into from
Apr 17, 2019

Conversation

bash
Copy link

@bash bash commented Apr 13, 2019

Resolves #2729.

There are a couple of things that I'm not sure if I did them correctly:

  • annotate-snippets doesn't have an "internal error" annotation type. I'm currently printing "internal" as the annotation id (where the error id usually is in rustc):
    Screenshot 2019-04-13 at 21 49 49
  • Does rustfmt have any guarantees about the public API when used as a library? Because I removed fancy_print and the Display impl from FormatReport both of which were pub.
  • Is there a particular reason that "warning" was previously printed in red? It is now printed in yellow:
    Screenshot 2019-04-13 at 21 54 41

Copy link
Contributor

@scampi scampi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution ! Can you add few docs about the new functions you added ?

Regarding fancy_print I believe its targeted use was within the crate. If used as a library, I think one would rely on the report struct, not on its pretty output (unless for debugging).
However, given we reached 1.0, we should strive for not breaking public API. @topecongiro what do you think ? What about marking that method as deprecated ?

src/format_report_formatter.rs Outdated Show resolved Hide resolved
src/format_report_formatter.rs Outdated Show resolved Hide resolved
@topecongiro
Copy link
Contributor

@scampi
There is an RFC which defines rustfmt stability (cc rust-lang/rust#54504). In the RFC, API braeking change is defined as follows:

A change that could cause a dependent crate not to build, could break a script using the executable, or breaks specification-level formatting compatibility. A formatting change in this category would be frustrating even for occasional users.

and this includes changes to the public API to rustfmt as a library. Hence, removing fancy_print and the Display impl from FormatReport is a breaking change and requires a major version update.

@topecongiro
Copy link
Contributor

@bash This looks great, thank you! I really like the change you brought with this PR, so using them internally is perfectly fine. But for the reason I noted in the above comment, could you put back the removed fancy_print and Display impl for FormatReport?

@scampi
Copy link
Contributor

scampi commented Apr 16, 2019

@topecongiro thanks for pointing this out

@bash
Copy link
Author

bash commented Apr 16, 2019

@scampi
Thanks for the review! I addressed both of your code comments.
I still need to improve/add comments to the new functions added in this PR.

@bash
Copy link
Author

bash commented Apr 16, 2019

@topecongiro
I re-implemented fancy_print and the Display impl for FormatReport using the new FormatReportFormatter. I've marked both as deprecated, because I want to encourage consumers of the library to choose the new API. Is that a viable solution?

Copy link
Contributor

@scampi scampi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Are you OK with the changes @topecongiro ?

@topecongiro
Copy link
Contributor

Awesome, thank you!

@topecongiro topecongiro merged commit 3dc625c into rust-lang:master Apr 17, 2019
@bash bash deleted the use-library-for-errors branch April 23, 2019 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a library for emitting errors
3 participants