[INFRA] LGTM recommendation: Unused local variable #853
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DimitriPapadopoulos
force-pushed
the
lgtm
branch
from
9f54004
to
d643e8e
Compare
sappelhoff
reviewed
Aug 23, 2021
The value assigned to local variable 'data' is never used.
DimitriPapadopoulos
force-pushed
the
lgtm
branch
from
14586f9
to
bc96981
Compare
sappelhoff
approved these changes
Aug 23, 2021
sappelhoff
reviewed
Aug 23, 2021
pdf_build_src/process_markdowns.py
Outdated
| @@ -16,7 +16,7 @@ | |||
| import numpy as np | |||
|
|
|||
| sys.path.append("../tools/") | |||
| from schemacode import macros | |||
| from schemacode import macros # lgtm [py/unused-import] | |||
There was a problem hiding this comment.
is importing macros somehow required? or could we also remove this line? cc @tsalo
There was a problem hiding this comment.
I believe its is required because we later call eval("macros.some_function(...)"):
https://github.com/bids-standard/bids-specification/blob/dd377ae/pdf_build_src/process_markdowns.py#L457
Thus, the lgtm suppression comment is quite useful, it reminds maintainers of this atypical call of macros. functions. I don't know if there's an equivalent noqa error code, but note that noqa support in LGTM.com is buggy (github/codeql#6517).
There was a problem hiding this comment.
thanks! Yes, I have used ... # noqa: F401 before. It that works, it'd be preferable to the LGTM thing, I think.
Perhaps even a comment like: ... # noqa: F401 --> it's used in "eval" below
There was a problem hiding this comment.
A simple noqa would be taken into account by LGTM.com, but not noqa: F401. What about these options?
# noqa (used in "eval" call below)
# noqa: F401 (noqa because used in "eval" call below)
# noqa: F401 lgtm [py/unused-import] (used in "eval" call below)
There was a problem hiding this comment.
+1 for option 2, if that works, else option 1
There was a problem hiding this comment.
👍 OK for option 2, I believe the LGTM regex will catch the second lone noqa :-)
There was a problem hiding this comment.
Option 2 is too long, and I have just noticed the second noqa requires spaces around it.
I've pushed option 1.
DimitriPapadopoulos
force-pushed
the
lgtm
branch
from
bc96981
to
b6e32a8
Compare
Import of 'macros' is not used.
DimitriPapadopoulos
force-pushed
the
lgtm
branch
from
b6e32a8
to
bb6065a
Compare
|
Thanks @DimitriPapadopoulos |
Closed
DimitriPapadopoulos
added a commit
to DimitriPapadopoulos/bids-specification
that referenced
this pull request
Aug 27, 2021
Previous commit bb6065a from bids-standard#853 does not work around the LGTM alert. We attempt to silence the LGTM alert using an lgtm.yml file, and use standard Flake8 noqa suppression comments to document the issue at hand.
DimitriPapadopoulos
added a commit
to DimitriPapadopoulos/bids-specification
that referenced
this pull request
Sep 1, 2021
Previous commit bb6065a from bids-standard#853 does not work around the LGTM alert. We attempt to silence the LGTM alert using an lgtm.yml file, and use standard Flake8 noqa suppression comments to document the issue at hand.
DimitriPapadopoulos
added a commit
to DimitriPapadopoulos/bids-specification
that referenced
this pull request
Sep 13, 2021
Previous commit bb6065a from bids-standard#853 does not work around the LGTM alert. We attempt to silence the LGTM alert using an lgtm.yml file, and use standard Flake8 noqa suppression comments to document the issue at hand.
DimitriPapadopoulos
added a commit
to DimitriPapadopoulos/bids-specification
that referenced
this pull request
Sep 13, 2021
Previous commit bb6065a from bids-standard#853 does not work around the LGTM alert. We attempt to silence the LGTM alert using an lgtm.yml file, and use standard Flake8 noqa suppression comments to document the issue at hand.
DimitriPapadopoulos
added a commit
to DimitriPapadopoulos/bids-specification
that referenced
this pull request
Sep 13, 2021
Previous commit bb6065a from bids-standard#853 does not work around the LGTM alert. We attempt to silence the LGTM alert using an lgtm.yml file, and use standard Flake8 noqa suppression comments to document the issue at hand.
DimitriPapadopoulos
added a commit
to DimitriPapadopoulos/bids-specification
that referenced
this pull request
Sep 13, 2021
Previous commit bb6065a from bids-standard#853 does not work around the LGTM alert. We attempt to silence the LGTM alert using an lgtm.yml file, and use standard Flake8 noqa suppression comments to document the issue at hand. Veuillez saisir le message de validation pour vos modifications. Les lignes
DimitriPapadopoulos
added a commit
to DimitriPapadopoulos/bids-specification
that referenced
this pull request
Sep 13, 2021
Previous commit bb6065a from bids-standard#853 does not work around the LGTM alert. We attempt to silence the LGTM alert using an lgtm.yml file, and use standard Flake8 noqa suppression comments to document the issue at hand.
DimitriPapadopoulos
added a commit
to DimitriPapadopoulos/bids-specification
that referenced
this pull request
Sep 13, 2021
Previous commit bb6065a from bids-standard#853 does not work around the LGTM alert. We attempt to silence the LGTM alert using an lgtm.yml file, and use standard Flake8 noqa suppression comments to document the issue at hand.
DimitriPapadopoulos
added a commit
to DimitriPapadopoulos/bids-specification
that referenced
this pull request
Sep 13, 2021
Previous commit bb6065a from bids-standard#853 does not work around the LGTM alert. We attempt to silence the LGTM alert using an lgtm.yml file, and use standard Flake8 noqa suppression comments to document the issue at hand.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The value assigned to local variable 'data' is never used.