src,tools: rewrite check-imports and fix linter errors#6105
Uh oh!
There was an error while loading. Please reload this page.
Conversation
thefourtheye added tools 
Fishrock123 commented Apr 7, 2016
concept seems good, can't comment on the python though |
tools/check-imports.py Outdated
There was a problem hiding this comment.
Can you use two-space indent in this file?
Fishrock123 commented Apr 7, 2016
Looks like you missed one though, unless the CI is outdated. |
thefourtheye commented Apr 7, 2016
@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. |
tools/check-imports.py Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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= (numfornuminxrange(10)) >>>all(item*item<20foritemingen) 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= (numfornuminxrange(10)) >>>all([item*item<20foritemingen]) False>>>list(gen) []Wrote more about it in this StackOverflow answer
bnoordhuis commented Apr 7, 2016
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.) |
thefourtheye commented Apr 8, 2016
@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. |
tools/check-imports.py Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
thefourtheye commented Apr 12, 2016
As this patch adds a new linter task to the build process, ccing @nodejs/build |
thefourtheye commented Apr 20, 2016
If there are no objections, I'll land this in two days. cc @nodejs/collaborators |
indutny commented Apr 20, 2016
LGTM, finally! |
jasnell commented Apr 20, 2016
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 <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]>
This patch fixes all the linter errors. PR-URL: #6105 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]>
thefourtheye commented Apr 25, 2016
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 <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]>
This patch fixes all the linter errors. PR-URL: nodejs#6105 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]>
This patch fixes all the linter errors. PR-URL: #6105 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]>
This patch fixes all the linter errors. PR-URL: #6105 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins commented Jun 1, 2016
@thefourtheye should this be backported? |
thefourtheye commented Jun 2, 2016
@thealphanerd There is no harm in backporting this, but I am not sure if it is absolutely necessary :( |
MylesBorins commented Jun 2, 2016
@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 |
7 participants
Checklist
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/