Add protection against invalid image path declarations for add
#247
Conversation
This is a bit painful, because datalad-next parameter validation is not available, but worth adding given the issue. Closes datalad#150
|
Code Climate has analyzed commit 7938498 and detected 0 issues on this pull request. View more on Code Climate. |
|
Test failures reveal that internal use of the parameter is also inconsistent with its documented semantics. |
| @@ -199,6 +199,16 @@ def __call__(name, url=None, dataset=None, call_fmt=None, image=None, | |||
| purpose='add container') | |||
| runner = WitlessRunner() | |||
|
|
|||
| if image is not None: | |||
| if op.isabs(image): | |||
There was a problem hiding this comment.
Docstring does say relative so I guess it is good. I just wanted to raise a comment that on general we are ok with providing absolute paths everywhere and I think we (or git) normalize them into relative if needed... so it would've been more consistent to just normalize absolute path to correct relative here
|
Appveyor shows now failing test_run_subdataset_install because path isn't absolute... I guess somewhat reflects on my comment above |
Yes, this is what I was referring to in #247 (comment) The Q is: what is wrong? The docs or the test? I interpret you statements above as: the docs should be adjusted (any path that points into the dataset), and the test relaxed accordingly. |
|
After looking at this again, I decided to close this PR. There would be a significant effort resulting in the serial parameter validation that the I think this is better approached once that or an equivalent functionality is available to this extension. |
This is a bit painful, because datalad-next parameter validation is not available, but worth adding given the issue.
Closes #150