src,tools: rewrite check-imports and fix linter errors by thefourtheye · Pull Request #6105 · nodejs/node · GitHub

Checklist

tests and code linting passes
the commit message follows commit guidelines

Affected core subsystem(s)

src, tools

Description of change

Commit 1

As it is, check-install.sh does not show more helpful error messages,
and supporting various shells could be a problem. This patch rewrites
the same in Python.

Commit 2

This patch simply enables check-imports.py in the linting process

Commit 3

Fix all the linter errors

cc @bnoordhuis

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

concept seems good, can't comment on the python though

Can you use two-space indent in this file?

@bnoordhuis Done!

Looks like you missed one though, unless the CI is outdated.

File "src/handle_wrap.cc" does not use "Boolean"
Makefile:632: recipe for target 'cpplint' failed

@Fishrock123 Hmmm, weird! I ran the linter locally before pushing. Anyway, I fixed it and the new CI Run (https://ci.nodejs.org/job/node-test-pull-request/2211/) completed linting successfully.

Can you remove the trailing blank line? Aside, you don't strictly need the square brackets in the line above but it doesn't hurt either.

@bnoordhuis Actually, that is necessary. all and any short-circuit. They return True and False the moment they meet an item doesn't meet the criteria. Passing a generator expression will leave the rest of them unexecuted. For example,

>>> gen = (num for num in xrange(10))
>>> all(item * item < 20 for item in gen)
False
>>> list(gen)
[6, 7, 8, 9]

But when we use List Comprehension, it executes all of them and only their results are used with all and any. For example,

>>> gen = (num for num in xrange(10))
>>> all([item * item < 20 for item in gen])
False
>>> list(gen)
[]

Wrote more about it in this StackOverflow answer

LGTM with a style nit. I'm somewhat ambivalent about the rewrite to python, the new script is two or three times as long as the old one (if you exclude the copyright boilerplate.)

@bnoordhuis The other reason I rewrote it in Python was because, if this is included in the linter, all the development environments are supposed to have sed, grep, sort and etc. That may not be available in all the environments I guess. I am okay with retaining the shell script itself, if possible.

What would you think of using filename instead of file? I’d find that more a bit readable + it doesn’t shadow the file built-in.

@addaleax I don't know how I missed that. Thanks for catching it :-) Fixed it now.

As it is, check-install.sh does not show more helpful error messages, and supporting various shells could be a problem. This patch rewrites the same in Python.

This patch simply enables check-imports.py in the linting process

As this patch adds a new linter task to the build process, ccing @nodejs/build

If there are no objections, I'll land this in two days. cc @nodejs/collaborators

LGTM, finally!

LGTM

As it is, check-install.sh does not show more helpful error messages, and supporting various shells could be a problem. This patch rewrites the same in Python. This patch also enables check-imports.py in the linting process PR-URL: #6105 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This patch fixes all the linter errors. PR-URL: #6105 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 2c480bd and 6781d91. Fixed one more linter error and a missed change from file to file_name.

As it is, check-install.sh does not show more helpful error messages, and supporting various shells could be a problem. This patch rewrites the same in Python. This patch also enables check-imports.py in the linting process PR-URL: nodejs#6105 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This patch fixes all the linter errors. PR-URL: nodejs#6105 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

As it is, check-install.sh does not show more helpful error messages, and supporting various shells could be a problem. This patch rewrites the same in Python. This patch also enables check-imports.py in the linting process PR-URL: #6105 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This patch fixes all the linter errors. PR-URL: #6105 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

@thefourtheye should this be backported?

@thealphanerd There is no harm in backporting this, but I am not sure if it is absolutely necessary :(

@thefourtheye this does not land cleanly. I'm adding the dont-land label. Feel free to send a backport PR if you would like, but I don't see it as pressing

thefourtheye added tools Issues and PRs related to the tools directory. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 7, 2016

bnoordhuis reviewed Apr 7, 2016
View reviewed changes

thefourtheye force-pushed the fix-imports branch from 6803533 to 06d15a3 Compare April 7, 2016 19:45

bnoordhuis reviewed Apr 7, 2016
View reviewed changes

addaleax reviewed Apr 8, 2016
View reviewed changes

thefourtheye added 3 commits April 12, 2016 21:49

tools: rewrite check-install.sh in python

9a5306a

As it is, check-install.sh does not show more helpful error messages, and supporting various shells could be a problem. This patch rewrites the same in Python.

tools: enable check-imports.py in linter

c62876c

This patch simply enables check-imports.py in the linting process

src: fix check-imports.py linter errors

e6e37d3

thefourtheye force-pushed the fix-imports branch from 06d15a3 to e6e37d3 Compare April 12, 2016 16:20

thefourtheye closed this Apr 25, 2016

thefourtheye deleted the fix-imports branch April 25, 2016 13:57

This was referenced May 17, 2016

handle_wrap: expose {un}refed check to JS #5834

Merged

Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check" #6546

Merged

evanlucas mentioned this pull request May 17, 2016

2016-05-17, Version 6.2.0 (Stable) #6810

Merged

Fishrock123 mentioned this pull request May 19, 2016

Discuss backporting HandleWrap#hasRef() in a Minor nodejs/Release#108

Closed

MylesBorins mentioned this pull request Jun 2, 2016

Audit commits not found on v4.x-staging #7116

Closed

MylesBorins added the dont-land-on-v4.x label Jun 2, 2016

src,tools: rewrite check-imports and fix linter errors by thefourtheye · Pull Request #6105 · nodejs/node · GitHub

Uh oh!

Uh oh!

src,tools: rewrite check-imports and fix linter errors - #6105

src,tools: rewrite check-imports and fix linter errors#6105thefourtheye wants to merge 3 commits intonodejs:masternodejs/node:masterfrom thefourtheye:fix-importsthefourtheye/io.js:fix-importsCopy head branch name to clipboard

Conversation

thefourtheye commented Apr 7, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123 commented Apr 7, 2016

Uh oh!

bnoordhuis Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Apr 7, 2016

Uh oh!

thefourtheye commented Apr 7, 2016

Uh oh!

bnoordhuis Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Apr 7, 2016

Uh oh!

thefourtheye commented Apr 8, 2016

Uh oh!

addaleax Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye Apr 12, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Apr 12, 2016

Uh oh!

thefourtheye commented Apr 20, 2016

Uh oh!

indutny commented Apr 20, 2016

Uh oh!

jasnell commented Apr 20, 2016

Uh oh!

thefourtheye commented Apr 25, 2016

Uh oh!

MylesBorins commented Jun 1, 2016

Uh oh!

thefourtheye commented Jun 2, 2016

Uh oh!

MylesBorins commented Jun 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

src,tools: rewrite check-imports and fix linter errors#6105
thefourtheye wants to merge 3 commits into
nodejs:masterfrom
thefourtheye:fix-imports