-
Notifications
You must be signed in to change notification settings - Fork 111
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
BF: AnnotatePaths #1881
BF: AnnotatePaths #1881
Conversation
datalad/interface/annotate_paths.py
Outdated
# TODO: That `path` in following line seems to be incorrect; | ||
# we are operating on `r` or `p` respectively. Guess it | ||
# should be `r`. | ||
# Figure out, whether or not to report on the resolved one: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. But also cannot tell immediately what is the correct fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'll go for the unresolved one for now, since we probably want to report on the original input. If that turns out to be wrong, we'll notice, I guess. :-)
datalad/interface/annotate_paths.py
Outdated
for r in requested_paths: | ||
if not lexists(r['path'] if isinstance(r, dict) else r): | ||
preserved_paths.append(r) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I aggree with the approach, but this should be a simple list comprehension, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Will change.
from itertools import chain | ||
# re-append the preserved paths: | ||
requested_paths = chain(requested_paths, iter(preserved_paths)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't these two plain (non-nested) lists? If so, why not just requested_paths.extend(preserved_paths)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. After modification detection requested_paths
is a generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew there would be something. ACK.
Codecov Report
@@ Coverage Diff @@
## master #1881 +/- ##
===========================================
+ Coverage 48.83% 85.72% +36.89%
===========================================
Files 263 265 +2
Lines 29660 31630 +1970
===========================================
+ Hits 14484 27115 +12631
+ Misses 15176 4515 -10661
Continue to review full report at Codecov.
|
163b537
to
00dc333
Compare
I guess those buildbots have some trouble again, @yarikoptic ? |
Crap sorry... I did restart firewall, but then did restart buildbots, thought it is all ok, will check |
@bpoldrack can you trigger the buildbot tests please? |
…ble paths. (Closes datalad#1880)
00dc333
to
08718be
Compare
Just amended empty to trigger them, but ... |
FTR: Happens in other branches, too. |
Fwiw the datasets.datalad.org was fixed, so now it should be genuine failures |
Nope. The only failure in Travis is a "connection broken" when trying to download PyGithub ;-) |
This pull request fixes #1880.
To be more specific:
If 'modified' was set, AnnotatePaths failed to report on unavailable paths.
Please have a look @mih . You might want to say something about the approach and the comments I left in the code.