Skip to content
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

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

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
7 participants
@thefourtheye
Copy link
Contributor

commented Apr 7, 2016

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/

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 7, 2016

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

@@ -0,0 +1,43 @@
#!/usr/bin/env python

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 7, 2016

Member

Can you use two-space indent in this file?

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 7, 2016

Author Contributor

@bnoordhuis Done!

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 7, 2016

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

@thefourtheye thefourtheye force-pushed the thefourtheye:fix-imports branch Apr 7, 2016

@thefourtheye

This comment has been minimized.

Copy link
Contributor Author

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.

@bnoordhuis

View changes

tools/check-imports.py Outdated
return valid

sys.exit(0 if all([is_valid(file) for file in glob.iglob('src/*.cc')]) else 1)

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 7, 2016

Member

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.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 8, 2016

Author Contributor

@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

@bnoordhuis

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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.

@addaleax

View changes

tools/check-imports.py Outdated
import sys


def do_exist(file, lines, imported):

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 8, 2016

Member

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.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 12, 2016

Author Contributor

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

thefourtheye added some commits Apr 7, 2016

tools: rewrite check-install.sh in python
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
This patch simply enables check-imports.py in the linting process

@thefourtheye thefourtheye force-pushed the thefourtheye:fix-imports branch to e6e37d3 Apr 12, 2016

@thefourtheye

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2016

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

@thefourtheye

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

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

@indutny

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

LGTM, finally!

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

LGTM

thefourtheye added a commit that referenced this pull request Apr 25, 2016

tools: rewrite check-install.sh in python
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>

thefourtheye added a commit that referenced this pull request Apr 25, 2016

src: fix check-imports.py linter errors
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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2016

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

@thefourtheye thefourtheye deleted the thefourtheye:fix-imports branch Apr 25, 2016

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

tools: rewrite check-install.sh in python
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>

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

src: fix check-imports.py linter errors
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>

jasnell added a commit that referenced this pull request Apr 26, 2016

tools: rewrite check-install.sh in python
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>

jasnell added a commit that referenced this pull request Apr 26, 2016

src: fix check-imports.py linter errors
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>

evanlucas added a commit that referenced this pull request May 17, 2016

src: fix check-imports.py linter errors
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>
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jun 1, 2016

@thefourtheye should this be backported?

@thefourtheye

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2016

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

@MylesBorins

This comment has been minimized.

Copy link
Member

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

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.