Skip to content
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

ENH: Complain like Git if to-be-removed sibling is unknown #4257

Merged
merged 2 commits into from
Mar 9, 2020

Conversation

adswa
Copy link
Member

@adswa adswa commented Mar 7, 2020

fixes #4249

@codecov
Copy link

codecov bot commented Mar 7, 2020

Codecov Report

Merging #4257 into master will decrease coverage by 0.68%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4257      +/-   ##
==========================================
- Coverage   89.43%   88.74%   -0.69%     
==========================================
  Files         275      282       +7     
  Lines       36182    36891     +709     
==========================================
+ Hits        32358    32739     +381     
- Misses       3824     4152     +328
Impacted Files Coverage Δ
datalad/distribution/siblings.py 76.2% <33.33%> (-0.4%) ⬇️
datalad/api.py 75.86% <0%> (-17.25%) ⬇️
datalad/plugin/wtf.py 82.2% <0%> (-6.78%) ⬇️
datalad/cmdline/main.py 77.4% <0%> (-2.52%) ⬇️
datalad/interface/run_procedure.py 90.74% <0%> (-1.86%) ⬇️
datalad/interface/__init__.py 100% <0%> (ø) ⬆️
...talad/distributed/tests/test_create_sibling_ria.py 100% <0%> (ø) ⬆️
datalad/support/_lru_cache2.py
datalad/distributed/ora_remote.py 19.1% <0%> (ø)
datalad/distributed/tests/test_ria_git_remote.py 62.16% <0%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32921eb...60c4788. Read the comment docs.

@adswa
Copy link
Member Author

adswa commented Mar 7, 2020

As far as I can see, the failing tests don't seem to be related to the change in this PR...

@@ -301,6 +301,10 @@ def __call__(
@staticmethod
def custom_result_renderer(res, **kwargs):
from datalad.ui import ui
# should we attempt to remove an unknown sibling, complain like Git does
if res['status'] == 'notneeded' and res['action'] == 'remove-sibling':
ui.message('fatal: No such remote: {name}'.format(**dict(res)))
Copy link
Contributor

@kyleam kyleam Mar 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks like an improvement to me. I have the following suggestions, some nit-picks:

  • You could just use **res instead of passing res to dict.

  • While I think we should complain in the spirit of git remote rm, I think it'd be better to not complain exactly like it. In particular, I'd say we should drop "fatal: " and replace "remote" with "sibling".

  • The message should include some indication of the path so that recursive operation gives a clearer message:

    % datalad subdatasets
    subdataset(ok): subds (dataset)
    % datalad siblings -r remove -s nonexistent
    fatal: No such remote: nonexistent
    fatal: No such remote: nonexistent
    

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these comments, I think I have addressed them now.

@kyleam
Copy link
Contributor

kyleam commented Mar 7, 2020

As far as I can see, the failing tests don't seem to be related to the change in this PR...

Yep, this is unlikely to cause issues given that functionally this is just adding a ui.message call. I've gone through the failures and those all look like ones we're seeing elsewhere.

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thx! I took the liberty to add the standard warning color to the output.

@adswa
Copy link
Member Author

adswa commented Mar 9, 2020

uhhh, cool, color!

@mih mih merged commit 3c4f4c6 into datalad:master Mar 9, 2020
@mih
Copy link
Member

mih commented Mar 9, 2020

Travis failure is unrelated and there is no chance that the type of change I made would fail in this test run only.

@adswa adswa deleted the enh-sibmessage branch March 9, 2020 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unlike Git, Datalad doesn't complain when removing an unknown sibling
3 participants