Address LGTM alerts #657
Address LGTM alerts #657
Conversation
Codecov Report
@@ Coverage Diff @@
## master #657 +/- ##
==========================================
+ Coverage 83.62% 83.65% +0.03%
==========================================
Files 59 59
Lines 5348 5352 +4
==========================================
+ Hits 4472 4477 +5
+ Misses 876 875 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
This pull request fixes 7 alerts when merging 2d09ba7 into 8aa9f7f - view on LGTM.com fixed alerts:
|
dandi/_version.py
Outdated
| @@ -504,7 +504,7 @@ def get_versions(): | |||
| # versionfile_source is the relative path from the top of the source | |||
| # tree (where the .git directory might live) to this file. Invert | |||
| # this to find the root from __file__. | |||
| for i in cfg.versionfile_source.split('/'): | |||
| for _ in cfg.versionfile_source.split('/'): | |||
There was a problem hiding this comment.
this is versioneer one, can we just skip it for LGTM?
meanwhile checked -- current master in versioneer already has this:
$> git grep -A1 'in cfg.versionfile_source.split'
src/git/long_get_versions.py: for _ in cfg.versionfile_source.split('/'):
src/git/long_get_versions.py- root = os.path.dirname(root)
so in principle we could just proceed, but still I think we should just skip this file
There was a problem hiding this comment.
I don't know how ignoring alerts works in LGTM, so I can't answer that.
There was a problem hiding this comment.
google has all the answers: https://lgtm.com/help/lgtm/customizing-file-classification
tools/validate-api-against-girder.py
Outdated
|
|
||
| from dandi.dandiapi import DandiAPIClient | ||
| from dandi.dandiset import APIDandiset | ||
| from dandi.girder import GirderCli |
There was a problem hiding this comment.
let's kill this file altogether -- girder is gone gone
There was a problem hiding this comment.
Should tools/instantiate-dandisets.py (which also uses Girder) be deleted as well?
|
This pull request fixes 7 alerts when merging ed93dad into 0fa24ee - view on LGTM.com fixed alerts:
|
|
This pull request fixes 7 alerts when merging b5aa3db into 0fa24ee - view on LGTM.com fixed alerts:
|
dandi/due.py
Outdated
| @@ -55,7 +55,7 @@ def _donothing_func(*args, **kwargs): | |||
|
|
|||
|
|
|||
| try: | |||
| from duecredit import due, BibTeX, Doi, Url, Text | |||
| from duecredit import due, Doi # lgtm [py/unused-import] | |||
There was a problem hiding this comment.
Let's skip this file as well from lgtm checks - it is a stub which comes from duecredit and better be left unmodified
There was a problem hiding this comment.
BTW, please submit a PR to duecredit for due.py so anyone who copies it also would get those pragma annotations
There was a problem hiding this comment.
Do you want me to revert the changes to our local due.py?
There was a problem hiding this comment.
yes, to due.py and _version.py and list them to be excluded in to be added lgtm.yml per https://lgtm.com/help/lgtm/customizing-file-classification
There was a problem hiding this comment.
oh I see you added already lgtm.yml -- great
|
This pull request fixes 7 alerts when merging 7efcb32 into 0fa24ee - view on LGTM.com fixed alerts:
|
|
This pull request fixes 6 alerts when merging da1073f into 0fa24ee - view on LGTM.com fixed alerts:
|
|
Thank you @jwodder ! Let's proceed. I feel we might like to pacify this LGTM bot to not add comments -- adding to the checks list is IMHO sufficient. |
Some alerts are false positives and thus are not addressed by this PR.