Better hint when a sibling is provided without --to #5726

adswa · 2021-06-09T14:30:30Z

This is an attempt to fix #5712. If

a path is given,
but not --to,
(a part of) the path matches a known sibling,
and no matching files are found,

push reports "impossible" instead of "notneeded", and issues a hint to the user.

Before:

After:

codecov · 2021-06-09T14:40:38Z

Codecov Report

Merging #5726 (3e3d6bd) into master (0b0e7fe) will decrease coverage by 1.94%.
The diff coverage is 88.00%.

❗ Current head 3e3d6bd differs from pull request most recent head a46f21e. Consider uploading reports for the commit a46f21e to get more accurate results

@@            Coverage Diff             @@
##           master    #5726      +/-   ##
==========================================
- Coverage   90.63%   88.69%   -1.95%     
==========================================
  Files         308      305       -3     
  Lines       42168    42153      -15     
==========================================
- Hits        38220    37387     -833     
- Misses       3948     4766     +818

Impacted Files	Coverage Δ
datalad/local/add_readme.py	`92.98% <ø> (ø)`
datalad/core/distributed/push.py	`92.51% <66.66%> (-0.98%)`	⬇️
datalad/core/distributed/clone.py	`92.07% <100.00%> (+0.11%)`	⬆️
datalad/core/distributed/tests/test_clone.py	`97.34% <100.00%> (+0.01%)`	⬆️
datalad/core/distributed/tests/test_push.py	`98.93% <100.00%> (-0.22%)`	⬇️
datalad/local/tests/test_add_readme.py	`100.00% <100.00%> (ø)`
datalad/plugin/wtf.py	`0.00% <0.00%> (-100.00%)`	⬇️
datalad/plugin/addurls.py	`0.00% <0.00%> (-100.00%)`	⬇️
datalad/plugin/no_annex.py	`0.00% <0.00%> (-100.00%)`	⬇️
datalad/plugin/add_readme.py	`0.00% <0.00%> (-100.00%)`	⬇️
... and 79 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 0b0e7fe...a46f21e. Read the comment docs.

bpoldrack · 2021-06-10T08:52:34Z

datalad/core/distributed/push.py

@@ -229,7 +229,13 @@ def __call__(
                    if sr
                    else 'No targets configured in dataset.'))
            return
-
+        potential_remote = False
+        if not to and path:


May be that should be relevant only, if the path doesn't actually exist?

good point. I moved it down into the if not matched_anything section.

Fixes 5712

bpoldrack

Messaging is fine that way. I'm not convinced of the result change, though. What's specified remains (technically) a path that couldn't be matched. I wouldn't know why this should yield impossible just because it happens to (also) be a remote name, while it's notneeded if it's an arbitrary not matched path.

adswa · 2021-06-10T12:35:26Z

Messaging is fine that way. I'm not convinced of the result change, though. What's specified remains (technically) a path that couldn't be matched. I wouldn't know why this should yield impossible just because it happens to (also) be a remote name, while it's notneeded if it's an arbitrary not matched path.

I oriented myself at how the case of a push without --to is handled, which also yields an "impossible" result.

bpoldrack · 2021-06-10T12:52:00Z

I oriented myself at how the case of a push without --to is handled, which also yields an "impossible" result.

I see. But that's not (necessarily) the case here. The target could be auto-detectable (one existing remote only). In fact, if it wasn't we would fail impossible before (the yield from _push above your changes) and not get to that part of the code.
So, I'd stay in-line with the other if not matched_anything-result.

bpoldrack · 2021-06-10T12:54:05Z

Arguably, we could distinguish the cases where not matched actually means - doesn't exist from "no changes" and yield impossible in the former case, but this should then apply generally, not just in the case where the not existing path happens to also be a remote name.

mih

Thx! I think this will improve UX!

I left a few comments on some possible optimizations.

mih · 2021-06-14T08:14:21Z

datalad/core/distributed/push.py

@@ -229,7 +229,6 @@ def __call__(
                    if sr
                    else 'No targets configured in dataset.'))
            return
-


Noooo... my favorite empty line...why did it have to die....life is not fair...it feels so meaningless... ;-)

I will bring it back to life ;-)

datalad/core/distributed/push.py

Co-authored-by: Michael Hanke <michael.hanke@gmail.com>

…target

…enderer

adswa · 2021-06-14T11:44:38Z

I have reused the hints framework that push's custom result renderer uses now. Here is how it looks like:

adjust test

adswa · 2021-06-14T20:19:42Z

The failure of one AppVeyor run seems unrelated

bpoldrack · 2021-06-15T11:05:26Z

The failure of one AppVeyor run seems unrelated

Failed during setup. Restarted.

mih

This looks good to me now! Thx!

DOC: typo in parameter description

4926733

adswa force-pushed the ux-pushtarget branch from 0fc4232 to 2554869 Compare June 9, 2021 15:52

bpoldrack reviewed Jun 10, 2021

View reviewed changes

UX: When a remote is specified as a path, provide a hint about --to

1a3ad7d

Fixes 5712

adswa force-pushed the ux-pushtarget branch from 2554869 to 1a3ad7d Compare June 10, 2021 09:28

bpoldrack reviewed Jun 10, 2021

View reviewed changes

mih reviewed Jun 14, 2021

View reviewed changes

adswa and others added 4 commits June 14, 2021 11:53

Confine number of paths to one when checking if a path is a sibling

c60eac4

Co-authored-by: Michael Hanke <michael.hanke@gmail.com>

improve hint message

cfe311c

Merge branch 'ux-pushtarget' of github.com:adswa/datalad into ux-push…

9bc3d96

…target

Reuse the 'hints' setup in result rendering of push's custom result r…

5ede193

…enderer

adswa added 2 commits June 14, 2021 14:30

UX: Render hints after result reporting, and shorten hint message

7d12d76

adjust test

When no path matches, but it resembles a sibling, return early

a46f21e

adswa force-pushed the ux-pushtarget branch from 9e3f956 to a46f21e Compare June 14, 2021 12:30

mih mentioned this pull request Jun 14, 2021

Dry run option for clean command #5738

Merged

mih approved these changes Jun 21, 2021

View reviewed changes

mih merged commit 2e2027b into datalad:master Jun 21, 2021

Better hint when a sibling is provided without --to #5726

Better hint when a sibling is provided without --to #5726

adswa commented Jun 9, 2021

codecov bot commented Jun 9, 2021 •

edited

bpoldrack Jun 10, 2021

adswa Jun 10, 2021

bpoldrack left a comment

adswa commented Jun 10, 2021

bpoldrack commented Jun 10, 2021

bpoldrack commented Jun 10, 2021

mih left a comment

mih Jun 14, 2021

adswa Jun 14, 2021

adswa commented Jun 14, 2021

adswa commented Jun 14, 2021

bpoldrack commented Jun 15, 2021

mih left a comment

Better hint when a sibling is provided without --to #5726

Better hint when a sibling is provided without --to #5726

Conversation

adswa commented Jun 9, 2021

codecov bot commented Jun 9, 2021 • edited

Codecov Report

bpoldrack Jun 10, 2021

Choose a reason for hiding this comment

adswa Jun 10, 2021

Choose a reason for hiding this comment

bpoldrack left a comment

Choose a reason for hiding this comment

adswa commented Jun 10, 2021

bpoldrack commented Jun 10, 2021

bpoldrack commented Jun 10, 2021

mih left a comment

Choose a reason for hiding this comment

mih Jun 14, 2021

Choose a reason for hiding this comment

adswa Jun 14, 2021

Choose a reason for hiding this comment

adswa commented Jun 14, 2021

adswa commented Jun 14, 2021

bpoldrack commented Jun 15, 2021

mih left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 9, 2021 •

edited