-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
tools: make the lint rules depend on tools #16881
Conversation
@echo "Running C++ linter..." | ||
@$(PYTHON) tools/cpplint.py $? | ||
tools/.cpplintstamp: $(LINT_CPP_FILES) tools/cpplint.py tools/check-imports.py | ||
@$(PYTHON) tools/cpplint.py $(LINT_CPP_FILES) |
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.
Is there a way to retain the $?
? It provides a nice speed-up in the common case when only some files changed.
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 have not checked thoroughly but I think the argument in #16581 (comment) applies to the build/include_what_you_use
rule of cpplint.py.
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.
Doesn't really apply, IMO. The build fails if a deleted file is included.
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.
In the JS scenario, other alternatives include "change what named exports a file has" which would cause linter failures in any unchanged file that imported from it - deleting a file was just one example.
Ping @joyeecheung |
@BridgeAR Thanks, I am going to spend some time to work out #16881 (comment) (probably next weekend) |
Ping again |
I don't think I have seen problems with changes to cpplint.py since #16581 lands. I have not found a way to retain the |
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
tools
Somewhat related to #16581 (comment) , although this is not related to JS linting. The idea is if we update remark or cpplint.py, then we should re-run the linters so the new changes apply.