-
Notifications
You must be signed in to change notification settings - Fork 110
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
RF: Implement new install/get API #613
Conversation
Codecov Report
@@ Coverage Diff @@
## master #613 +/- ##
==========================================
+ Coverage 87.48% 87.68% +0.19%
==========================================
Files 216 219 +3
Lines 19912 20369 +457
==========================================
+ Hits 17421 17861 +440
- Misses 2491 2508 +17
Continue to review full report at Codecov.
|
how is it going? I would be excited to try new install and get combination ;) |
annex_get_opts=annex_get_opts) | ||
|
||
@staticmethod | ||
@datasetmethod(name='add') |
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.
name='get'
?
2 similar comments
recursion_limit=recursion_limit) | ||
if p_ds is None: | ||
raise FileNotInRepositoryError( | ||
msg="{0} not in dataset.".format(p)) |
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.
Or just a warning that this will be ignored?
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.
May be. Same for not existing files?
Observation:
All of the above symlinks already point to existing content. |
Somewhat overwhelmed by the verbosity on standard talkative level:
|
Re observation on identical content: Yes. Fixed already (partially), but not pushed yet. Result reporting is a topic for hangout. I have questions ... ;-) |
resolved_datasets.get(p_ds.path, []) + [p] | ||
|
||
# TODO: Change behaviour of Dataset: Make subdatasets singletons to | ||
# always get the same object referencing a certain subdataset. |
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 would worry about that later whenever we actually run into the problem... for now I think the most important is to get get
and install
working correctly
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.
Actually, "the problem" is that it currently leads to a less clean and straight implementation of API. But I agree - getting them working correctly again is the more important thing right now.
yes edit: we discussed that before, agreed that it is useful, and we had it implemented (and I thought we had a test for that). So imho it should stay implemented edit2: later we might want to make it up to configuration variable to do such installations in general or not, and ask user upon initial invocation |
Well, if you say so, I will add it. |
ENH: more concise result renderer for install
…rchy was not an annex. (Closes datalad#864, datalad#862)
and tune it a little bit
@@ -262,6 +270,9 @@ def __call__( | |||
relpath(opj(ds_path, local_results[i]['file']), ds.path) | |||
|
|||
global_results.extend(local_results) | |||
|
|||
if not found_an_annex: | |||
lgr.warning("Found no annex. Could not perform any get operation.") |
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 am ok with such approach instead of mine... just fix up the test... I forgot also to improve that test by making an annex subdataset and testing actual recursive call on super pure-git dataset... we really need those tests
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.
just cherry pick that commit ENHing single_or_plural
please.
ok -- I will leave you alone for now ;)
I think that the merge of this PR is upon us -- basic real life testing identified a few issue, but it is quite good already. so I will mark it as for milestone 0.3 ;-) |
(cherry picked from commit 5a6b9a5)
So we are just 1 quick fix away from the merge! ;) whoohooo -- great job @bpoldrack! It feels like the beast could be usable now! But I would really appreciate if you give a test yourself on our |
# Keep original in debug output: | ||
lgr.debug("Original failure:{0}" | ||
"{1}".format(linesep, exc_str(e))) | ||
return None |
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 -- not None. Remember -- "fulfilling the promise!" ;) and that is why tests failed as well btw
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.
Sure, None
! ;-)
I think you misread it. If there is an installed dataset at the target we return it. But if it is something else we return None
since we neither installed a dataset nor is it already there.
But there's something else wrong with it. Condition conflicts with URL guessing, since an empty directory is okay, but could still fail in an attempt to guess the correct URL.
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.
Hopefully works now ;-)
# (TODO: eventually check for being the one, that this | ||
# is about) | ||
if current_dataset.is_installed(): | ||
lgr.info("{0} appears to be installed already.") |
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.
forgotten .format()
?
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.
Fixed.
break | ||
else: | ||
lgr.warning("Target {0} already exists and is not an " | ||
"installed dataset. Skipped.") |
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.
again forgotten format or I am not aware of some magic? (in python3.5 e.g. you could do smth like r"{path} blah"
which would use local var path
)
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.
You're right. Committed to fast.
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.
Fixed.
BF: Tested wrong condition get was unnecessarily recursivly called from within install
Damn it. OSX failing ... |
# is about) | ||
if current_dataset.is_installed(): | ||
lgr.info("{0} appears to be installed already." | ||
"".format(current_dataset)) |
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.
ha - "".format
-- cute... ;) thought myself about what to do in such cases ;)
No actual description. Just referencing things ...
Closes #553
Closes #834
Closes #724
Closes #693
Closes #787
install
viaget
get
of non Annex repo (super)datasets (#613) get -r should just not bother callingget
of non Annex repo (super)datasets #864