Skip to content

Salvage parts of #5695 #6440

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

Merged
merged 2 commits into from
Feb 10, 2022
Merged

Salvage parts of #5695 #6440

merged 2 commits into from
Feb 10, 2022

Conversation

mih
Copy link
Member

@mih mih commented Feb 10, 2022

This PR salvages all commits from #5695 that are still relevant, except for a5d554b which is presently being debated in #6438 (as one of two options).

Changelog

🏠 Internal

  • Common command arguments are now uniformly and exhaustively passed to result renderers and filters for decision making. Previously, the presence of a particular argument depended on the respective API and circumstances of a command call.

When run() was first added, it used to take the exit code from the
CommandError raised by a failed command and create a fresh
CommandError with the exit code.  That hasn't been the case since
3817fb4 (ENH: LEt `run` spit out the original full-detail error,
2018-04-25).
eval_results() combines the positional and keyword arguments of a
command's __call__() into an OrderedDict.  This has been passed to
result filters since 141a55f (RF: Make positionals also available
as kwargs to result_filter, 2017-04-24) and to custom result renderers
since 6267129 (BF: Result renderers never got to see all args of a
command call, 2019-12-23).

Whether the dictionary of arguments contains eval_params parameters
depends on how the command is called.  From the command line,
return_type, result_renderer, and result_xfm are always set by
Interface.call_from_parser(), while on_failure is set if --on-failure
is given and result_filter if --report-status is.  From the Python
API, the dictionary includes only eval_params parameters that are
explicitly specified.

To renderers and filters, the above distinctions should not matter.
Pass them a dictionary that always includes the effective values of
the eval_params parameters so that they can reliably act on these
parameters.
@codeclimate
Copy link

codeclimate bot commented Feb 10, 2022

Code Climate has analyzed commit fa4b34d and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Security 1

View more on Code Climate.

@mih mih added the semver-patch Increment the patch version when merged label Feb 10, 2022
@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #6440 (fa4b34d) into master (b868dbc) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6440      +/-   ##
==========================================
+ Coverage   89.87%   89.97%   +0.10%     
==========================================
  Files         348      348              
  Lines       43764    43859      +95     
==========================================
+ Hits        39331    39464     +133     
+ Misses       4433     4395      -38     
Impacted Files Coverage Δ
datalad/core/local/run.py 97.84% <ø> (ø)
datalad/interface/tests/test_utils.py 100.00% <100.00%> (ø)
datalad/interface/utils.py 95.50% <100.00%> (ø)
datalad/customremotes/tests/test_archives.py 89.28% <0.00%> (-0.65%) ⬇️
datalad/consts.py 100.00% <0.00%> (ø)
datalad/interface/common_cfg.py 100.00% <0.00%> (ø)
datalad/local/tests/test_addurls.py 99.58% <0.00%> (+<0.01%) ⬆️
datalad/core/local/tests/test_save.py 98.11% <0.00%> (+0.09%) ⬆️
datalad/support/tests/test_annexrepo.py 97.97% <0.00%> (+0.13%) ⬆️
datalad/distribution/tests/test_create_sibling.py 77.57% <0.00%> (+0.15%) ⬆️
... and 14 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 b868dbc...fa4b34d. Read the comment docs.

Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

Looks straightforward to me indeed

@mih
Copy link
Member Author

mih commented Feb 10, 2022

Thx!

@mih mih merged commit aebb5ce into datalad:master Feb 10, 2022
@mih mih deleted the rf-run branch February 10, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants