BF: interface: Drop repeated custom_result_summary_renderer call #4463
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kyleam
commented
Apr 29, 2020
datalad/interface/tests/test_base.py
Outdated
| from datalad.core.local.status import Status | ||
|
|
||
| # This regression test depends on the command having a custom summary | ||
| # renderer that renders output. status() having this method doesn't |
There was a problem hiding this comment.
If we keep this test: "custom summary renderer that renders output" sounds silly. I should rephrase it. I was trying to make the point that this test depends on the custom summary renderer outputting something, which isn't a given (e.g., the call in this test wouldn't have any output if annex data weren't added).
Codecov Report
@@ Coverage Diff @@
## maint #4463 +/- ##
==========================================
+ Coverage 89.63% 89.68% +0.04%
==========================================
Files 275 275
Lines 37093 37102 +9
==========================================
+ Hits 33249 33275 +26
+ Misses 3844 3827 -17
Continue to review full report at Codecov.
|
mih
approved these changes
Apr 30, 2020
kyleam
added a commit
to kyleam/datalad
that referenced
this pull request
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 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
Since 8e7950d (ENH: Add the ability for custom result summary rendering, 2018-12-05), command-line calls that request tailored output from commands with a custom_result_summary_renderer() method show repeated lines: $ datalad -f tailored status --annex 1 annex'd file (4.0 B recorded total size) 1 annex'd file (4.0 B recorded total size) With that commit, there are three custom_result_summary_renderer() calls: * one in setup_parser(), added by f558ada (NF: Result summary renderer [...], 2017-03-21) * another in the return_type!="generator" path of eval_func(), again added by f558ada. * a newer one in the return_type="generator" path of eval_func(), added by 8e7950d While the latest addition exposed the issue from the command line, it is actually the older call in setup_parser() that seems most problematic. The two original call sites meant that calls from the command line honored custom_result_summary_renderer() while Python API calls with return_type="generator" did not. So, drop the call in setup_parser(), which is now handled by the return_type="generator" path from 8e7950d.
|
@mih Thanks for taking a look. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since 8e7950d (ENH: Add the ability for custom result summary
rendering, 2018-12-05), command-line calls that request tailored
output from commands with a custom_result_summary_renderer() method
show repeated lines:
With that commit, there are three custom_result_summary_renderer()
calls:
one in setup_parser(), added by f558ada (NF: Result summary
renderer [...], 2017-03-21)
another in the return_type!="generator" path of eval_func(), again
added by f558ada.
a newer one in the return_type="generator" path of eval_func(),
added by 8e7950d
While the latest addition exposed the issue from the command line, it
is actually the older call in setup_parser() that seems most
problematic. The two original call sites meant that calls from the
command line honored custom_result_summary_renderer() while Python API
calls with return_type="generator" did not. So, drop the call in
setup_parser(), which is now handled by the return_type="generator"
path from 8e7950d.
I'm not sure if it's worth carrying the added regression test.