build,python: update flake8 rules #25614

refack · 2019-01-21T16:58:42Z

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

node/Makefile

Lines 1294 to 1297 in f698386

    
           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), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2019-01-21T16:58:45Z

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2320/pipeline

cclauss

LGTM

joyeecheung · 2019-01-21T18:48:31Z

We skip markdown and JS linting for the fixtures folder, I don't see why we should not do the same for python?

cclauss · 2019-01-21T19:02:54Z

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 name in __all__
F823: local variable name referenced before assignment
E901: SyntaxError or IndentationError
E999: SyntaxError -- failed to compile a file into an Abstract Syntax Tree

joyeecheung

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.

cclauss · 2019-01-21T19:25:04Z

What is upstream? We might not want to maintain these scripts but we do not want them to fail on our users.

thefourtheye · 2019-01-22T11:42:13Z

@joyeecheung 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?

joyeecheung · 2019-01-22T12:42:14Z

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.

refack · 2019-01-22T16:04:40Z

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.

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.

refack · 2019-01-22T16:42:36Z

blocked by: #25639

refack · 2019-01-22T18:10:54Z

Rebased on master and latest WPT.

Now this PR restores linting of test/fixtures to validate we don't track broken code.

joyeecheung · 2019-01-22T19:09:16Z

@refack I am still -1 to lint test/fixtures at all, no matter it's python or not.

thefourtheye · 2019-01-23T13:12:58Z

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.

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?

cclauss · 2019-01-23T13:36:39Z

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.

joyeecheung · 2019-01-23T13:50:49Z

@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.

Relevant changes where updated

nodejs-github-bot · 2019-04-13T16:22:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22416/

.flake8

nodejs-github-bot · 2019-04-13T22:14:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22421/

nodejs-github-bot · 2019-04-13T22:32:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22422/

* Tree-factor location of some *.py files for easy demarcation of areas to exclude. PR-URL: nodejs#25614 Reviewed-By: Sakthipriyan Vairamani <[email protected]>

PR-URL: nodejs#25614 Reviewed-By: Sakthipriyan Vairamani <[email protected]>

refack · 2019-04-14T00:34:55Z

Landed in 914d6c9...1fc4255

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]>

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]>

PR-URL: nodejs#25614 Reviewed-By: Sakthipriyan Vairamani <[email protected]> (cherry picked from commit a16a0fe)

refack requested a review from joyeecheung January 21, 2019 16:58

nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 21, 2019

This was referenced Jan 21, 2019

ansible: python3 for freebsd (for linting) nodejs/build#1644

Merged

Can 'make lint-py PYTHON=python3' be a manditory Jenkins test? nodejs/build#1631

Closed

cclauss approved these changes Jan 21, 2019

View reviewed changes

thefourtheye approved these changes Jan 21, 2019

View reviewed changes

joyeecheung previously requested changes Jan 21, 2019

View reviewed changes

refack mentioned this pull request Jan 22, 2019

build: do not lint python scripts under test/fixtures #25639

Merged

2 tasks

refack added the blocked PRs that are blocked by other issues or PRs. label Jan 22, 2019

refack added build Issues and PRs related to build files or the CI. python PRs and issues that require attention from people who are familiar with Python. labels Jan 22, 2019

refack force-pushed the float-patch-wpt branch from 36f30fc to 7c7d86d Compare January 22, 2019 18:07

refack changed the title ~~test,tools: float python3 patch on WPT fixtures~~ build,python: restore python linting of test/fixtures Jan 22, 2019

refack removed the blocked PRs that are blocked by other issues or PRs. label Jan 22, 2019

cclauss mentioned this pull request Jan 29, 2019

build: ongoing list of actions for Python 3 compatibility #25789

Closed

63 tasks

refack force-pushed the float-patch-wpt branch from 7c7d86d to 419441e Compare April 11, 2019 14:45

cclauss reviewed Apr 13, 2019

View reviewed changes

.flake8 Outdated Show resolved Hide resolved

refack force-pushed the float-patch-wpt branch from 8a7402f to 1d1c80f Compare April 13, 2019 22:13

refack force-pushed the float-patch-wpt branch from 1d1c80f to 7ac5415 Compare April 13, 2019 22:31

refack added 3 commits April 13, 2019 20:32

tools: python: update flake8 rules

0befda6

* Tree-factor location of some *.py files for easy demarcation of areas to exclude. PR-URL: nodejs#25614 Reviewed-By: Sakthipriyan Vairamani <[email protected]>

tools: python: activate more flake8 rules

a16a0fe

PR-URL: nodejs#25614 Reviewed-By: Sakthipriyan Vairamani <[email protected]>

tools: python: ignore instead of select flake8 rules

1fc4255

PR-URL: nodejs#25614 Reviewed-By: Sakthipriyan Vairamani <[email protected]>

refack force-pushed the float-patch-wpt branch from 7ac5415 to 1fc4255 Compare April 14, 2019 00:34

refack removed the request for review from joyeecheung April 14, 2019 00:35

refack merged commit 1fc4255 into nodejs:master Apr 14, 2019

refack deleted the float-patch-wpt branch April 14, 2019 00:43

refack removed their assignment Apr 14, 2019

ryzokuken mentioned this pull request Apr 23, 2019

tools: update tools/license-builder.sh #27362

Merged

BaochengSu pushed a commit to BaochengSu/node that referenced this pull request Oct 20, 2020

tools: python: activate more flake8 rules

6daf7e9

PR-URL: nodejs#25614 Reviewed-By: Sakthipriyan Vairamani <[email protected]> (cherry picked from commit a16a0fe)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build,python: update flake8 rules #25614

build,python: update flake8 rules #25614

refack commented Jan 21, 2019 •

edited

Loading

nodejs-github-bot commented Jan 21, 2019

cclauss left a comment

joyeecheung commented Jan 21, 2019

cclauss commented Jan 21, 2019 •

edited

Loading

joyeecheung left a comment •

edited

Loading

cclauss commented Jan 21, 2019 •

edited

Loading

thefourtheye commented Jan 22, 2019

joyeecheung commented Jan 22, 2019 •

edited

Loading

refack commented Jan 22, 2019

refack commented Jan 22, 2019

refack commented Jan 22, 2019

joyeecheung commented Jan 22, 2019

thefourtheye commented Jan 23, 2019

cclauss commented Jan 23, 2019

joyeecheung commented Jan 23, 2019 •

edited

Loading

nodejs-github-bot commented Apr 13, 2019

nodejs-github-bot commented Apr 13, 2019

nodejs-github-bot commented Apr 13, 2019

refack commented Apr 14, 2019

	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

build,python: update flake8 rules #25614

build,python: update flake8 rules #25614

Conversation

refack commented Jan 21, 2019 • edited Loading

Checklist

nodejs-github-bot commented Jan 21, 2019

cclauss left a comment

Choose a reason for hiding this comment

joyeecheung commented Jan 21, 2019

cclauss commented Jan 21, 2019 • edited Loading

joyeecheung left a comment • edited Loading

Choose a reason for hiding this comment

cclauss commented Jan 21, 2019 • edited Loading

thefourtheye commented Jan 22, 2019

joyeecheung commented Jan 22, 2019 • edited Loading

refack commented Jan 22, 2019

refack commented Jan 22, 2019

refack commented Jan 22, 2019

joyeecheung commented Jan 22, 2019

thefourtheye commented Jan 23, 2019

cclauss commented Jan 23, 2019

joyeecheung commented Jan 23, 2019 • edited Loading

nodejs-github-bot commented Apr 13, 2019

nodejs-github-bot commented Apr 13, 2019

nodejs-github-bot commented Apr 13, 2019

refack commented Apr 14, 2019

refack commented Jan 21, 2019 •

edited

Loading

cclauss commented Jan 21, 2019 •

edited

Loading

joyeecheung left a comment •

edited

Loading

cclauss commented Jan 21, 2019 •

edited

Loading

joyeecheung commented Jan 22, 2019 •

edited

Loading

joyeecheung commented Jan 23, 2019 •

edited

Loading