-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Cleanup of #[derive(Diagnostic)] attribute parsers
#151657
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
9f11d3b to
e38bc5a
Compare
This comment has been minimized.
This comment has been minimized.
e38bc5a to
2e8347a
Compare
|
cc @davidtwco, @TaKO8Ki |
hm, do we still need this notification?
yep, i think so too
i'll take a closer look tomorrow, but i'm pretty sure that's all fine <3 |
Afaik these notifications are for people who want to know if certain directories change, whether it is needed is mostly for them to decide. There is no further expectations such as waiting for their review. Actually just added myself to the list, cause I'm now also interested in changes of these files #151800 |
oh, that's nice, thanks for keeping an eye on it |
This PR does a lot of refactoring on the implementation of
#[derive(Diagnostic)]. It should have no observable effect other than error messages for incorrect usage of the attributes. In general, I think the error messages got better.This PR can be reviewed commit by commit, each commit passes the tests.
Start parsing
#[diagnostic]usingsyn'sparse_args_withfunction instead ofparse_nested_meta. This improves error messages and prepares for the new syntax needed for Tracking issue for MCP 959: Remove the fluent files #151366 which cannot be parsed usingparse_args_with.Same as above but for
#[subdiagnostic]Removes the
no_spanoption of#[suggestion], which there were no tests for and which seems to have been unused. If needed again in the future, this can be re-added pretty easily, but I find that unlikely.Removes the
HasFieldMaptrait, because I don't really see the point of having a trait "has a field map" if we can just pass the fieldmap itself instead.r? @Kivooeo
(Thanks for reviewing my PRs so far :3)