Lint: improve subtree exclusion #29965
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
The Win64 GHA test failure looks unrelated. If you want, you can split up 54b2115bb848e1aad8a86aae858de63a851235ce into its own pull request, as it is an independent improvement. But up to you. |
|
I think I will leave it in here since the motivation for 54b2115 is mostly the increased number of lint checks only accessible through the lint test runner. But I will split it out if anyone has an objection, or if this PR stalls. |
|
Windows CI appears to have timed out, rebased to get a fresh CI run. |
|
Looks like the conflicting #30034 is close(r) to merge, if you want to review it |
This is now merged. Can you rebase here? |
|
Rebased to address merge conflict |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
|
Any thoughts on splitting the first commit out into a separate pull? Seems to be a requested feature by three people so far, but I am not sure about bundling it for review with the other changes here. |
You are right, split out the first commit into: #30219 |
0fcbfdb Support running individual lint checks (David Gumberg) Pull request description: This PR was split out from #29965: Adds support for running individual tests in the rust lint suite by passing `--lint=LINT_TO_RUN` to the lint runner. This PR also adds a corresponding help message. When running with `cargo run`, arguments after a double dash (`--`) are passed to the binary instead of the cargo command. For example, in order to run the linter check that tabs are not used as whitespace: ```console cd test/lint/test_runner && cargo run -- --lint=tabs_whitespace ``` ACKs for top commit: maflcko: ACK 0fcbfdb achow101: ACK 0fcbfdb marcofleon: Tested ACK 0fcbfdb. Ran `cargo run` with various of the individual tests and with bad input. Also ran it with no arguments. Everything works as expected and help message looks good. Tree-SHA512: 48fe4aa9fbb2acef5f8e3c17382ae22e0e350ae6ad9aeeb1a3c0a9192de98809f98728e32b8db24a36906ace999e35626ebd6cb2ca05f74146d21e9b6fb14615
This makes all the lint checks written in rust exclude subtrees, and refactors the exclusion lists into a separate file.
Make the include guard linter print all warnings before exiting, and reuse common exclusion logic to improve subtree exclusion (crypto/ctaes was previously not excluded)
Rewrites the 'includes' linter to make use of common exclusion logic, improving subtree exclusion. (crypto/ctaes was previously not excluded)
This commit rewrites the spelling linter to reuse common
exclusion logic and improves subtree exclusion. ('crypto/ctaes' was
previously not excluded)
This commit rewrites the locale linter to reuse common exclusion logic and improves subtree exclusion (src/crc32c was previously not excluded)
Rewrites the python encoding lint check to make use of common exclusion logic.
|
Rebased over master since #30219 was merged. |
| ]) | ||
| .args(["grep", "std::filesystem", "--"]) | ||
| .args(["--", "./src"]) | ||
| .args(exclude::get_pathspecs_exclude_std_filesystem()) |
There was a problem hiding this comment.
nit in 9d1423a: Not sure about moving a single string literal that is used in a single function to a new module into a new function.
args allows for chaining, so if you want to append more args to the existing ones, my recommendation would be to just call args one more time with the args that you want to append.
I think the lint tests can be easy to understand and pragmatic, and adding too many layers may not be useful to understand them.
(This will also make the diff smaller, and is thus easier to review)
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Are you still working on this? |
|
Closing this for now, it's up for grabs, if I have a chance to work on this again soon I'll split into smaller PR's |
This PR improves subtree exclusion by rewriting lint checks to reuse common exclusion logic. Most lint checks previously did not exclude
crypto/ctaes, the locale linter did not excludesrc/crc32Makes the include guards lint check print all missing include guards before exiting.
Supercedes #29744