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

tools: make the lint rules depend on tools #16881

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected 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.

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 8, 2017
@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)
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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.

@BridgeAR
Copy link
Member

Ping @joyeecheung

@joyeecheung
Copy link
Member Author

@BridgeAR Thanks, I am going to spend some time to work out #16881 (comment) (probably next weekend)

@joyeecheung joyeecheung added the wip Issues and PRs that are still a work in progress. label Nov 26, 2017
@BridgeAR
Copy link
Member

BridgeAR commented Jan 5, 2018

Ping again

@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 15, 2018

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 $ and personally I prefer only running the linters on the files changed. If there is anything wrong, the linter CI will always catch that. I am going to close this now but if anyone still wants to pursue this, feel free to pick it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants