X Tutup
The Wayback Machine - https://web.archive.org/web/20190722125413/https://github.com/nodejs/node/pull/25614
Skip to content

/node

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

build,python: update flake8 rules #25614

Merged
merged 3 commits into from Apr 14, 2019
+97 −144

Conversation

Reviewers

@cclauss cclauss

@thefourtheye thefourtheye

@joyeecheung joyeecheung

Assignees
No one assigned
Labels
build python tools
Projects
None yet
Milestone
No milestone
5 participants
@refack @nodejs-github-bot @joyeecheung @cclauss @thefourtheye
@refack
Copy link
Member

commented Jan 21, 2019 •
edited

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:

  1. float a patch and wait for web-platform-tests/wpt#14973
  2. clean up directory of dead code
  3. 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

@refack refack requested a review from joyeecheung Jan 21, 2019

@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the test label Jan 21, 2019

This was referenced Jan 21, 2019

Merged
Open
@cclauss

cclauss approved these changes Jan 21, 2019
View changes

Copy link
Contributor

left a comment

LGTM
@thefourtheye

thefourtheye approved these changes Jan 21, 2019
View changes

@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jan 21, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Jan 21, 2019 •
edited

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

joyeecheung reviewed Jan 21, 2019
View changes

Copy link
Member

left a comment •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Jan 21, 2019 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Jan 22, 2019

@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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jan 22, 2019 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jan 22, 2019

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 refack referenced this pull request Jan 22, 2019

Merged

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

2 of 2 tasks complete

@refack refack added the blocked label Jan 22, 2019

@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jan 22, 2019

blocked by: #25639

@refack refack added build python labels Jan 22, 2019

@refack refack force-pushed the refack:float-patch-wpt branch from 36f30fc to 7c7d86d Jan 22, 2019

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

@refack refack removed the blocked label Jan 22, 2019

@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jan 22, 2019

Rebased on master and latest WPT.

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jan 22, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Jan 23, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Jan 23, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jan 23, 2019 •
edited

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

@cclauss cclauss referenced this pull request Jan 29, 2019

Open

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

7 of 15 tasks complete

@refack refack force-pushed the refack:float-patch-wpt branch from 7c7d86d to 419441e Apr 11, 2019

@refack refack dismissed joyeecheung’s stale review Apr 11, 2019

Relevant changes where updated

@refack refack requested a review from joyeecheung Apr 12, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Apr 13, 2019

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

cclauss reviewed Apr 13, 2019
View changes

Show resolved Hide resolved .flake8 Outdated

@refack refack force-pushed the refack:float-patch-wpt branch from 8a7402f to 1d1c80f Apr 13, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Apr 13, 2019

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

@refack refack force-pushed the refack:float-patch-wpt branch from 1d1c80f to 7ac5415 Apr 13, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Apr 13, 2019

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

refack added some commits Apr 11, 2019

@refack
tools: python: update flake8 rules 
* Tree-factor location of some *.py files for easy demarcation of
  areas to exclude.

PR-URL: #25614
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
0befda6
@refack
tools: python: activate more flake8 rules 
PR-URL: #25614
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
a16a0fe
@refack
tools: python: ignore instead of select flake8 rules 
PR-URL: #25614
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Loading status checks…
1fc4255

@refack refack force-pushed the refack:float-patch-wpt branch from 7ac5415 to 1fc4255 Apr 14, 2019

@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 14, 2019

Landed in 914d6c9...1fc4255

@refack refack removed the request for review from joyeecheung Apr 14, 2019

@refack refack merged commit 1fc4255 into nodejs:master Apr 14, 2019
1 of 2 checks passed

1 of 2 checks passed

Travis CI - Pull Request Build Errored
Details
Travis CI - Branch Build Passed
Details

@refack refack deleted the refack:float-patch-wpt branch Apr 14, 2019

@refack refack removed their assignment Apr 14, 2019

ryzokuken added a commit to ryzokuken/node that referenced this pull request Apr 23, 2019

@ryzokuken
tools: update tools/license-builder.sh 
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
Verified
This commit was signed with a verified signature.
ryzokuken Ujjwal Sharma
GPG key ID: 1FD3B47B83F46621 Learn about signing commits
8432ade

@ryzokuken ryzokuken referenced this pull request Apr 23, 2019

Merged

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

refack added a commit to ryzokuken/node that referenced this pull request Apr 23, 2019

@ryzokuken @refack
tools: update tools/license-builder.sh 
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>
fcb98bb

This was referenced Apr 23, 2019

Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed

targos added a commit that referenced this pull request Apr 27, 2019

@ryzokuken @targos
tools: update tools/license-builder.sh 
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>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
d62a324
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
X Tutup