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.

Sign up for GitHub

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

Jump to bottom

multiple issues with get output (in master) #4455

Closed
yarikoptic opened this issue Apr 28, 2020 · 2 comments · Fixed by #4471
Closed

multiple issues with get output (in master) #4455

yarikoptic opened this issue Apr 28, 2020 · 2 comments · Fixed by #4471
Assignees
@kyleam
Labels
fix-implemented A fix is available, but has not been merged or released, yet. UX user experience
Milestone
0.13.0

Comments

@yarikoptic
Copy link
Member

Wanted to check effects of #4454 on other commands and ran into a surprising new behavior:

$> datalad --version ; datalad get openneuro/ds000017/sub-1/ses-timepoint1/anat/sub-1_ses-timepoint1_inplaneT2.nii.gz
datalad 0.12.6.dev611
Tried to get 1 file that had no content yet. 1 (failed).
/home/yoh/datalad/openneuro/ds000017 [dataset] ... notneeded
/home/yoh/datalad/openneuro/ds000017/sub-1/ses-timepoint1/anat/sub-1_ses-timepoint1_inplaneT2.nii.gz [file] ... notneeded

whenever released version is more succinct:

$> datalad --version ; datalad get openneuro/ds000017/sub-1/ses-timepoint1/anat/sub-1_ses-timepoint1_inplaneT2.nii.gz
datalad 0.12.6.dev1
action summary:
  get (notneeded: 2)

so it is strange that

  • reports "Tried to get 1 file that had no content yet. 1 (failed)." although there is no such file and nothing really failed -- file does have content:
$> ls -Ll openneuro/ds000017/sub-1/ses-timepoint1/anat/sub-1_ses-timepoint1_inplaneT2.nii.gz
-r-------- 1 yoh yoh 661198 Apr 28 19:23 openneuro/ds000017/sub-1/ses-timepoint1/anat/sub-1_ses-timepoint1_inplaneT2.nii.gz
@yarikoptic yarikoptic added the UX user experience label Apr 28, 2020
@yarikoptic yarikoptic added this to the 0.13.0 milestone Apr 28, 2020
@mih
Copy link
Member

mih commented Apr 29, 2020

Closing #4429 assuming that a the new issue is better.

@kyleam kyleam self-assigned this Apr 29, 2020
@kyleam
Copy link
Contributor

kyleam commented Apr 29, 2020

I've started looking into this.

kyleam added a commit to kyleam/datalad that referenced this issue Apr 30, 2020
@kyleam
RF: get: Delete custom_result_summary_renderer
a1b6464 
The custom_result_summary_renderer for get() was added back in
f558ada (NF: Result summary renderer (more or less useful demo in
`get`), 2017-03-21).  At that time, a caller had to specify "tailored"
as the output format to see the custom summary output.  As of
b8b282d (ENH: enable custom summary result rendering also for
default rendering, 2020-03-24), the custom summary is now triggered
for the default renderer as well.

Here's an example of the custom summary when get() is called with two
files that are already present:

    Tried to get 2 files that had no content yet. 2 (failed).
    /tmp/dl-SckMiLB/two [file] ... notneeded
    /tmp/dl-SckMiLB/one [file] ... notneeded

As pointed out in dataladgh-4455, there are a few problems with this output:
1) the files do have content, 2) a "notneeded" result should not be
counted as a failure, and 3) the path reporting goes against the
current convention of reporting relative paths.  In addition, when
operating on a dataset (dataladgh-4429), there's a confusing message about 0
files having content.

To address these issues, we could either fix the handling in get's
custom_result_summary_renderer or delete the handler.  (Reverting
b8b282d to not call the custom summary renderer by default would
only hide away the problems again.)  Go with the second option because
it's unlikely that many users were regularly passing '-f tailored' to
see the custom summary renderer output.  If they were, we probably
would have seen a report about confusing output.  And even if the
problems above weren't enough to prompt a report, the duplicated
summary that has been happening since 0.11.2 (dataladgh-4463) seems unlikely
to have gone unnoticed and unreported, were anyone looking at this
output.

Closes datalad#4455.
Cc: @adswa
@kyleam kyleam mentioned this issue Apr 30, 2020
Merged
@kyleam kyleam added the fix-implemented A fix is available, but has not been merged or released, yet. label Apr 30, 2020
@mih mih closed this as completed in #4471 May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kyleam kyleam

Labels
fix-implemented A fix is available, but has not been merged or released, yet. UX user experience
Projects
None yet
Milestone
0.13.0
Development

Successfully merging a pull request may close this issue.

RF: get: Delete custom_result_summary_renderer kyleam/datalad
3 participants
@yarikoptic @mih @kyleam