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.

Sign up for GitHub

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

Jump to bottom

tools: check-imports.py false negatives #29226

Closed
bnoordhuis opened this issue Aug 20, 2019 · 5 comments
Closed

tools: check-imports.py false negatives #29226

bnoordhuis opened this issue Aug 20, 2019 · 5 comments
Labels
confirmed-bug Issues with confirmed bugs. tools Issues and PRs related to the tools directory.

Comments

@bnoordhuis
Copy link
Member

Refs: #29222

As that PR shows, there are some unused imports that tools/check-imports.py doesn't catch because:

  1. They're used in comments.
  2. The tool doesn't match at word boundaries (e.g. FromJust is considered a use of Just.)
@bnoordhuis bnoordhuis added confirmed-bug Issues with confirmed bugs. tools Issues and PRs related to the tools directory. labels Aug 20, 2019
@ayushmandey97
Copy link

Hey, I'm a bit new to opensource. Have some experience with Python and regex, and this looks interesting. Can I take this up?

@sam-github
Copy link
Contributor

@ayushmandey97 Yes, you are welcome to, thanks.

ayushmandey97 added a commit to ayushmandey97/node that referenced this issue Sep 30, 2019
@ayushmandey97
tools: fixed check-imports.py to ignore commented lines
3f7f560 
The script check-imports.py will now ignore the usage of module names in
commented lines. Additionally, while searching for the usage,
word boundaries will be used. Eg: 'Just' would not be considered in a
sentence containing 'FromJust'.

Fixes: nodejs#29226
@ayushmandey97 ayushmandey97 mentioned this issue Sep 30, 2019
Merged
2 tasks
@ayushmandey97
Copy link

I'll be adding support for multi-line comments as well.

There's one issue though, I added a dummy 'using' statement in one of the .cc files to check if an error was thrown, but the check-imports.py file ran without any log statements.
Any suggestions on how to test it in-case that's a wrong approach?

Another thing, since this is my first PR for Node, please feel free to let me know if I've made mistakes in the process or the code itself.

ayushmandey97 added a commit to ayushmandey97/node that referenced this issue Nov 11, 2019
@ayushmandey97
tools: Commented lines with spaces/tabs will also be ignored now
6f673dc 
    The script check-imports.py will now ignore commented lines with
spaces/tabs, and this fix also pointed the check to the correct src
folder.

    Fixes: nodejs#29226
ntantri pushed a commit to miqdigital/node that referenced this issue Nov 11, 2019
@ayushmandey97 @ntantri
tools: fixed check-imports.py to ignore commented lines
f9c9bc2 
The script check-imports.py will now ignore the usage of module names in
commented lines. Additionally, while searching for the usage,
word boundaries will be used. Eg: 'Just' would not be considered in a
sentence containing 'FromJust'.

Fixes: nodejs#29226
ntantri pushed a commit to miqdigital/node that referenced this issue Nov 11, 2019
@ayushmandey97 @ntantri
tools: Commented lines with spaces/tabs will also be ignored now
cc3ccc9 
    The script check-imports.py will now ignore commented lines with
spaces/tabs, and this fix also pointed the check to the correct src
folder.

    Fixes: nodejs#29226
@ayushmandey97
Copy link

ayushmandey97 commented Nov 24, 2019 •
edited
Loading

@sam-github the checks have been implemented. Please review once and let me know if any changes are required.

@bnoordhuis please have a look at the PR.

@richardlau richardlau mentioned this issue Apr 9, 2020
Closed
2 tasks
@richardlau richardlau mentioned this issue May 6, 2020
Merged
3 tasks
richardlau pushed a commit to richardlau/node-1 that referenced this issue May 28, 2020
@danbev @richardlau
src: remove unused using declarations
4a20cc9 
This commit removes the unused using declarations reported by lint-cpp.

PR-URL: nodejs#33268
Refs: nodejs#29226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
richardlau added a commit to richardlau/node-1 that referenced this issue May 28, 2020
@richardlau
tools: fix check-imports.py to match on word boundaries
05db682 
`check-imports.py` was missing some unused `using` statements as it
was not matching on word boundaries (e.g. `MaybeLocal` was considered
a use of `Local`). Fix that and add some unit tests (which required
the script to be renamed to drop the `-` so it could be imported into
the test script).

PR-URL: nodejs#33268
Refs: nodejs#29226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@ayushmandey97
Copy link

@sam-github looks like another PR for this issue got merged while I didn't get any feedback on the one I raised, was there a reason for this? Let me know if I missed something

codebytere pushed a commit that referenced this issue Jun 18, 2020
@danbev @codebytere
src: remove unused using declarations
ad97099 
This commit removes the unused using declarations reported by lint-cpp.

PR-URL: #33268
Refs: #29226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
codebytere pushed a commit that referenced this issue Jun 18, 2020
@richardlau @codebytere
tools: fix check-imports.py to match on word boundaries
a331a00 
`check-imports.py` was missing some unused `using` statements as it
was not matching on word boundaries (e.g. `MaybeLocal` was considered
a use of `Local`). Fix that and add some unit tests (which required
the script to be renamed to drop the `-` so it could be imported into
the test script).

PR-URL: #33268
Refs: #29226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
codebytere pushed a commit that referenced this issue Jun 30, 2020
@danbev @codebytere
src: remove unused using declarations
f0be9f5 
This commit removes the unused using declarations reported by lint-cpp.

PR-URL: #33268
Refs: #29226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
codebytere pushed a commit that referenced this issue Jun 30, 2020
@richardlau @codebytere
tools: fix check-imports.py to match on word boundaries
942aec2 
`check-imports.py` was missing some unused `using` statements as it
was not matching on word boundaries (e.g. `MaybeLocal` was considered
a use of `Local`). Fix that and add some unit tests (which required
the script to be renamed to drop the `-` so it could be imported into
the test script).

PR-URL: #33268
Refs: #29226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
codebytere pushed a commit that referenced this issue Jul 8, 2020
@richardlau @codebytere
tools: fix check-imports.py to match on word boundaries
3d05e3d 
`check-imports.py` was missing some unused `using` statements as it
was not matching on word boundaries (e.g. `MaybeLocal` was considered
a use of `Local`). Fix that and add some unit tests (which required
the script to be renamed to drop the `-` so it could be imported into
the test script).

PR-URL: #33268
Refs: #29226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
codebytere pushed a commit that referenced this issue Jul 8, 2020
@danbev @codebytere
src: remove unused using declarations
41d8796 
This commit removes the unused using declarations reported by lint-cpp.

PR-URL: #33268
Refs: #29226
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed-bug Issues with confirmed bugs. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@sam-github @bnoordhuis @ayushmandey97 @Mesteery