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

Add option to set minimum "mandatory" message level #182

Closed

Conversation

GuillaumeGomez
Copy link
Contributor

It is needed for clippy because we only want to set "error" message annotations as mandatory and not fail in case lower level are present in the file.

The reason is that currently, if a lower level message annotation is present, the "general level" of annotation becomes the lowest present in the test file, which is not something we want.

Needed for rust-lang/rust-clippy#11421.

@oli-obk
Copy link
Owner

oli-obk commented Nov 6, 2023

Why doesn't //@require_annotations_for_level ERROR satisfy your use case?

@GuillaumeGomez
Copy link
Contributor Author

It would need to be added to all clippy tests with mixed levels. It's a possibility, but I found it easier to set it directly for all tests (since it's already possible to use this option...) rather than adding it in hundred of existing ones. It will also makes things simpler for potential future new tests with mixed annotation levels if users don't need to know about this annotation (which, I precise, does exactly what we need).

@Alexendoo
Copy link
Contributor

Alexendoo commented Nov 6, 2023

While there are hundreds of test files with them in currently, almost all of them contain only NOTE: X implied by Y or NOTE: X on by default which would be better off being removed

We could bring it up at a meeting, but my impression is we will remove all the NOTE annotations before making it mandatory anyway. If there is a test that does want to use them it could use //@require_annotations_for_level, or it could just specify the other NOTEs in the file

@GuillaumeGomez
Copy link
Contributor Author

If it's only this kind of note messages, then better remove them indeed. Just would be sad to prevent adding other annotations but if it's what clippy devs want, fine by me. :)

@oli-obk
Copy link
Owner

oli-obk commented Nov 6, 2023

The way we should implement something like this in ui_test is to provide a default test config. That's a major refactoring that I probably have lots of opinions on, so I'd like to not just review impls but be part of the design and brain storm phase.

@GuillaumeGomez
Copy link
Contributor Author

We're not in a hurry so more discussions and brain storming are very welcome.

@oli-obk oli-obk mentioned this pull request Nov 7, 2023
@oli-obk
Copy link
Owner

oli-obk commented Nov 7, 2023

This is resolved by #183

@oli-obk oli-obk closed this Nov 7, 2023
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.

3 participants