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

RF: get: Delete custom_result_summary_renderer #4471

Merged
merged 1 commit into from May 2, 2020

Conversation

kyleam
Copy link
Collaborator

@kyleam kyleam commented Apr 30, 2020

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 gh-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 (gh-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 (gh-4463) seems unlikely
to have gone unnoticed and unreported, were anyone looking at this
output.

Closes #4455.
Cc: @adswa

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
@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #4471 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4471      +/-   ##
==========================================
+ Coverage   88.86%   88.91%   +0.04%     
==========================================
  Files         285      285              
  Lines       37903    37885      -18     
==========================================
+ Hits        33683    33684       +1     
+ Misses       4220     4201      -19     
Impacted Files Coverage Δ
datalad/distribution/get.py 96.09% <ø> (+1.47%) ⬆️
datalad/interface/results.py 97.67% <0.00%> (-0.78%) ⬇️
datalad/downloaders/tests/test_http.py 60.96% <0.00%> (+2.16%) ⬆️
datalad/downloaders/http.py 74.90% <0.00%> (+2.78%) ⬆️

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 6b6d671...a1b6464. Read the comment docs.

@adswa
Copy link
Member

adswa commented May 1, 2020

Thanks for cc'ing me, @kyleam, I didn't notice that b8b282d's side effect had led to this! I also prefer the second option.

mih
mih approved these changes May 2, 2020
Copy link
Member

@mih mih left a comment

I think this is a good step forward. Thx!

@mih mih merged commit a980a98 into datalad:master May 2, 2020
12 checks passed
@kyleam kyleam deleted the get-restrict-custom-summary branch May 19, 2020
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.

3 participants