lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner #29744
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.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
|
I think this needs a conceptual discussion if we want to move all Python code to Rust eventually. |
|
Concept ACK 🦀 |
I don't think moving all Python code to Rust makes sense. Simply due to the reason that reviewing such a large change is close to impossible. See also my reply in another thread: #29479 (comment) |
I think you are missing that |
|
Also, looking at this again, I think it could make sense to either add them to this pull, or correct the python list, and make them to use the python list. |
The tests are already split up into separate files and can be ported one at a time. We could pull in |
Sure, but I think the functional test framework itself is already 10k lines of code. Having to support two test frameworks that are supposed to behave similar or identical may already be too difficult. |
|
Previously the linters were separated into their own files and could be called separately as well. This is now moving to one big monolytic program, which is not great. The
If such a rewrite is not worth it on a big scale we should also not do it on a small scale. The value and effort scale up proportionally with the size of the change. And if we are concerned about resources I think we also should do this here either because a small distraction is a distraction as well. |
Good point. This should be trivial to fix. For example by having an environment variable to indicate which check to run. For example
I don't think this has changed. The linters written in bash were never run as part of "lint all", for example Generally, I think more important than the choice of language is to actually fix the issue that was intended to be solved. That is, #29744 (comment) and #29744 (comment) . Whether the fix is done in Python or Rust should be secondary, as long as the fix is done. |
Refactor lint runner into separate modules and use get_pathspecs_exclude* helpers everywhere.
Add support for passing `--lint=LINT_TO_RUN` to the rust lint runner.
|
I've substantially revised this PR to address @fjahr's feedback that the rust lint runner monofile was getting too big, and that it doesn't support running individual tests and @maflcko's suggestion to include I am just OK at regex, and had to do some tricky things to avoid importing the rust regex crate, so I ask any reviewers to give special care to all of the regex segments, especially the pattern in I feel that while whether the added review and complexity burden of the changes here and, broadly, of having another language for everyone to be knowledgeable of outweighs the benefits remains an open question, (I am concept +0) it is possible for further rewriting of the lint suite to be worthwhile without it being the case that moving all Python code to Rust should be a good idea. |
davidgumberg
changed the title
DrahtBot
changed the title
| } else { | ||
| Err("".to_string()) | ||
| } | ||
| } |
There was a problem hiding this comment.
In 1ffdf3f: Why? What is the point to put a single function consisting of a single if in a new file and module? I fail to see how more boilerplate than the actual logic is beneficial.
If your point is that large files shouldn't exist, then I tend to disagree. For example, doc/developer-notes.md is large and exists. Same for ./src/rpc/blockchain.cpp.
My preference would be to not move code, unless there is a reason.
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Are you still working on this? |
Yes, I was just waiting to see if there was any other feedback before putting all of the linter code back into a single file. I think a lot of the reason for the individual lint checks being placed in separate files previously was so that they could be run individually, but I feel that a single file that contains both the lint checks and the lint runner is a bit clunky. I would rather leave splitting up |
|
Concept NACK As I have said before, I think there needs to be a discussion about using more Rust just for the sake of using more Rust in the project. The only motivation given so far is that @maflcko said that it could be done. There is no benefit to doing this otherwise apparently and the biggest effect I see is that there will be fewer people able to contribute to that part of the code base. And this change alone will also take up a lot of review effort so it appears to be a clear negative, at least in the short to mid-term. |
Not sure where you get that from, but I never said that using more Rust for the sake of using more Rust can be done. I said something else:
|
"an alternative might be to rewrite the 3 lint checks to rust" #29479 (review) That is what @davidgumberg has given as the motivation in his description. It does not matter what you have said in other places because I have not been talking about that. I only talked about your comment that the author took as the motivation for this PR. |
|
That comment says |
Right, and that original PR was merged so that part does not apply here anymore, still this PR is still open.
Well, my impression is that the author got that impression and opened this PR because of it. |
My understanding is that the original PR didn't actually fix the issue, so the problem remains. For example, Let me know if I am missing something. If the issue is indeed fixed, this pull request can clearly be closed. In any case, it seems better to close this pull request and start a fresh one, if needed, which clearly explains the issue/problem in its own words, and how it is fixed. I don't think the discussion in this pull request, and the fact that the patch changed after opening, makes it easy to follow. |
Right, these are behavior changes that I did not see as part of the motivation of the original PR, as the title says it was a refactor. So I don't think this is something that was missed, it was just not the goal. It should be made more clear that this is the main motivation of this PR, to improve exclusion of subtrees. It should not be called a refactor in the title as it is now. |
|
Closing for now per previous discussion. Feel free to create a new pull request(s) with the behavior changes and an accurate description in your own words. Please try to avoid the use of the word "refactor" for bugfixes or behavior changes. |
Motivated by a comment in #29479, this draft PR refactors the lint runner into multiple modules, and refactors a few of the python linters to reuse common file excluding logic.
The include guards lint now prints all missing include guards before exiting, and all of the rewritten linters now exclude all subtrees, including
crypto/caes.The lint runner now supports running individual linters by passing
--lint=LINT_CHECK_TO_RUNto the lint runner. If usingcargo runarguments can be passed e.g.:I can see reasons why the benefits of this PR might be outweighed by the downsides so I'm just publishing this as a draft to explore if further rewriting/refactoring of the lint suite in rust is desirable, and if so, what the best approach is.