build,python: update flake8 rules by refack · Pull Request #25614 · nodejs/node (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation25 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
xrange does not exist in python3 hence it's lint blocking nodejs/build#1631, but it is not used in our codebase.
As I see it we have 3 options to unblock:
- float a patch and wait for python3 de-linting web-platform-tests/wpt#14973
- clean up directory of dead code
- ignore in
lint-py: PYTHONPATH=tools/pip $(PYTHON) -m flake8 . \ --count --show-source --statistics --select=E901,E999,F821,F822,F823 \ --exclude=.git,deps,lib,src,tools/*_macros.py,tools/gyp,tools/inspector_protocol,tools/jinja2,tools/markupsafe,tools/pip
Personally, I prefer option 1.
/CC @nodejs/testing @nodejs/python
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes- tests and/or benchmarks are included
- commit message follows commit guidelines
This was referenced
Jan 21, 2019
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We skip markdown and JS linting for the fixtures folder, I don't see why we should not do the same for python?
This is not a style violation, this is an undefined name that has the potential to raise a NameError at runtime that would halt the script. Why would we want to miss the opportunity to flatten such a "showstopper" issue?
E901,E999,F821,F822,F823 are the "showstopper" flake8 issues that can halt the runtime with a SyntaxError, NameError, etc. Most other flake8 issues are merely "style violations" -- useful for readability but they do not effect runtime safety.
- F821: undefined name
name - F822: undefined name
namein__all__ - F823: local variable name referenced before assignment
- E901: SyntaxError or IndentationError
- E999: SyntaxError -- failed to compile a file into an Abstract Syntax Tree
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should float patches in the test/fixtures/wpt folder, that would just unnecessarily complicate the WPT update workflow (what happens the next time we update encoding tests?). Instead we should just ignore the fixtures folder for linting, that's also what we do for JS and markdown since we simply do not own those files and it does not worth the effort to maintain floated patches there.
What is upstream? We might not want to maintain these scripts but we do not want them to fail on our users.
What is upstream? We might not want to maintain these scripts but we do not want them to fail on our users.
Upstream means https://github.com/web-platform-tests/wpt - the files under test/fixtures/wpt are subsets downloaded from there without modification.
web-platform-tests/wpt#14973 is merged in the upstream. Do we have a specific procedure to pull changes from wpt or this PR should be fine?
We do maintain a map of the commit sha for each subset, see the README of test/wpt: https://github.com/nodejs/node/tree/master/test/wpt#how-to-update-tests-for-a-module For encoding basically run git node wpt encoding from the project directory using node-core-utils and it's done - though it's possible that the update might pull in actual changes that fail the test suites, then either the test status file test/wpt/status/encoding needs to be updated to reflect the known issues, or the implementation needs to be fixed.
But I still think we should just ignore test/fixtures entirely in the linters - we ignore them in JS linters and markdown linters, I don't see a reason why we should lint the python scripts that we do not even use.
But I still think we should just ignore
test/fixturesentirely in the linters - we ignore them in JS linters and markdown linters, I don't see a reason why we should lint the python scripts that we do not even use.
As I see it, for CPP, markdown, and JS our linters are just for style, and we get actual code coverage with our test suite. For python we can lint for syntax and semantic issues. Also AFAIU we skip fixtures because fixing it is too noisy, while with this PR we can get lint coverage with minimal work.
PRs that are blocked by other issues or PRs.
label
Issues and PRs related to build files or the CI.
PRs and issues that require attention from people who are familiar with Python.
labels
test,tools: float python3 patch on WPT fixtures build,python: restore python linting of test/fixtures
PRs that are blocked by other issues or PRs.
label
Rebased on master and latest WPT.
Now this PR restores linting of test/fixtures to validate we don't track broken code.
@refack I am still -1 to lint test/fixtures at all, no matter it's python or not.
But I still think we should just ignore
test/fixturesentirely in the linters - we ignore them in JS linters and markdown linters, I don't see a reason why we should lint the python scripts that we do not even use.
Makes sense. I am okay with not linting Python scripts in test/fixtures.
Also AFAIU we skip fixtures because fixing it is too noisy, while with this PR we can get lint coverage with minimal work.
As we don't control upstream, if they add code which fails our python linter, we have to update it. That adds to our maintenance. We don't have to do that, right?
Is it best practice to be caught unaware when syntax errors come into our codebase?
We might not control upstream but they certainly seem to react quickly when we alert them to issues in their code. My sense is that the vigilance demonstrated in web-platform-tests/wpt#14973 is a way for us to give back to those projects that we rely on. They help us and we should be willing to help them.
@cclauss We always ignore deps in linters first and foremost, and those folders contains actual code that need to be executed when building Node.js - I don't see the need to be pedantic about something like test/fixtures when we don't even care about deps.
.flake8 Outdated Show resolved Hide resolved
- Tree-factor location of some *.py files for easy demarcation of areas to exclude.
PR-URL: nodejs#25614 Reviewed-By: Sakthipriyan Vairamani thechargingvolcano@gmail.com
PR-URL: nodejs#25614 Reviewed-By: Sakthipriyan Vairamani thechargingvolcano@gmail.com
PR-URL: nodejs#25614 Reviewed-By: Sakthipriyan Vairamani thechargingvolcano@gmail.com
Landed in 914d6c9...1fc4255
refack pushed a commit to ryzokuken/node that referenced this pull request
Update tools/license-builder.sh in order to work normally after jinja2 and markupsafe were moved from tools/ to tools/inspector_protocol/ in an earlier commit.
Refs: nodejs#25614
PR-URL: nodejs#27362 Reviewed-By: Michaël Zasso targos@protonmail.com Reviewed-By: Refael Ackermann refack@gmail.com Reviewed-By: Richard Lau riclau@uk.ibm.com
This was referenced
Apr 23, 2019
targos pushed a commit that referenced this pull request
Update tools/license-builder.sh in order to work normally after jinja2 and markupsafe were moved from tools/ to tools/inspector_protocol/ in an earlier commit.
Refs: #25614
PR-URL: #27362 Reviewed-By: Michaël Zasso targos@protonmail.com Reviewed-By: Refael Ackermann refack@gmail.com Reviewed-By: Richard Lau riclau@uk.ibm.com
BaochengSu pushed a commit to BaochengSu/node that referenced this pull request
PR-URL: nodejs#25614 Reviewed-By: Sakthipriyan Vairamani thechargingvolcano@gmail.com (cherry picked from commit a16a0fe)
Labels
Issues and PRs related to build files or the CI.
PRs and issues that require attention from people who are familiar with Python.
Issues and PRs related to the tools directory.