-
Notifications
You must be signed in to change notification settings - Fork 57
Consolidate warnings, suppression and deduplication #246
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
Conversation
b9e1d7f to
505e4d8
Compare
cli: Remove sample warning
505e4d8 to
584251b
Compare
error: Move W04 to `Warnings` error: Move W05 to `Warnings` error: Move W06 to `Warnings` error: Move W07 to `Warnings` error: Move W08 to `Warnings` error: Move W09 to `Warnings` error: Move W10 to `Warnings`
error: Move W11 to `Warnings` error: Move W12 to `Warnings` error: Move W13 to `Warnings` error: Move W14 to `Warnings` error: Deduplicate W15/W01 warnings error: Move W15 to `Warnings`
error: Move W17 to `Warnings`
error: Move W19 to `Warnings` error: Move W20 to `Warnings` error: Move W21 to `Warnings` error: Move W22 to `Warnings` error: Move W23 to `Warnings` error: Move W24 to `Warnings` error: Move W30 to `Warnings` error: Move W31 to `Warnings` error: Move W32 to `Warnings` error: Move uncoded Warning to `Warnings`
e6decd3 to
b8a2940
Compare
b8a2940 to
b7601c1
Compare
The `fancy` feature is mainly used for code annotation, that we don't use at the moment. Since we implement our own warning formatter, we don't actually require this feature
micprog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some minor things, but overall LGTM! I really like the updated output and the cleaner handling of suppressing warnings!
There are some warnings that should actually be help information for an error, which are currently noted as TODOs. Thanks for doing this, I think it's best addressed in an upcoming PR.
micprog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Warning collection
This PR collects the current warnings system into a monolithic
diagnostic::Warningsenum, which has the following benefits:Suppression handling improvements
Additionally, the current warning and error suppression system has been improved a bit. A global resp. static
Diagnosticshandler now takes care of suppression. It is initialized in the beginning with the user defined suppressed warnings. This cleans up the code sincesuppress_warningsdoes not need to be passed around anymore, and whether a warning is suppressed is checked automatically behind the scenes when a Warning is emitted. Further, theDiagnostichandler keeps track of emitted warnings and deduplicates them to avoid redundant warnings. It not only differentiates between the type of warning but also it's content (e.g.SomeWarningAbougPkgforpkg1is treaded as different thanSomeWarningAboutPkgforpkg2).Warning formatting
The new formatting of warnings and help messages is shown below:
(in reality the warnings are also colored properly)
New crates adoption
Two new crates were adopted to reduce boilerplate code and derive warning/help messages, error codes and severity from procedural macros:
mietteThis crate uses the
#[diagnostic]macro to implement traits for the specified enumeration variants that allows it to callhelp(),code()andseverity()on an enum variant.miettecan do a lot more and is mostly used for nice annotations of source code. In the case of Bender this is less relevant since it is not a compiler that reads source code. It could in theory be used to annotate wrongBender.ymlfiles, but this is a bit overkill at the moment, and our current YAML parser does not return positional information of fields in the source.thiserrorThe second crate uses the
#[error]macro to derive theDisplaytrait for the warning message. At the moment, we are not usingthiserrorto its full extend. It also implements automaticErrorconversion and chaining, which will be implemented in a separate PR to make the review easier.TODO:
miette, but suppression is impacted potentially.