Skip to content

tools: make the lint rules depend on tools #16881

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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.

# for free to join this conversation on GitHub. Already have an account? # 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