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

Refurbish eval_func() #3963

Merged
merged 7 commits into from Dec 30, 2019
Merged

Refurbish eval_func() #3963

merged 7 commits into from Dec 30, 2019

Conversation

mih
Copy link
Member

@mih mih commented Dec 22, 2019

This became a little more elaborate than desired, but I think it is useful nevertheless.

  • Added a test that demos how result_xfm broke result hooks #3962
  • Ripped out proc-pre/post feature based on feedback in #3908 in order to get a clean fix for #3962 (which fixes #3908)
  • Actually fix #3962
  • RF the decorator code, created helper functions (even if used just once) and removed duplicate variable to improve the subjective readability of the code
  • Result renderers now get to see all command args. Before it was dependent on how exactly a command was called: cmd(path='this') vs cmd('this') -- only the former case used to make it to the result renderer kwargs.

mih added 6 commits Dec 22, 2019
`result_xfm` specification disables result hooks
Based on feedback in datalad#3908

This paves the way to a clean fix of
datalad#3962

Fixes dataladgh-3908
And a bit of renaming to make code a bit more readable for me
Be more gentle to my aging brain
We only passed `kwargs`, not `allkwargs`. Only the latter would
capture the full picture.
@codecov
Copy link

@codecov codecov bot commented Dec 23, 2019

Codecov Report

Merging #3963 into master will decrease coverage by 0.05%.
The diff coverage is 96.22%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3963      +/-   ##
=========================================
- Coverage   89.75%   89.7%   -0.06%     
=========================================
  Files         271     271              
  Lines       36374   36328      -46     
=========================================
- Hits        32649   32587      -62     
- Misses       3725    3741      +16
Impacted Files Coverage Δ
datalad/interface/tests/test_base.py 100% <ø> (ø) ⬆️
datalad/interface/run_procedure.py 90.74% <ø> (+0.67%) ⬆️
datalad/interface/tests/test_run_procedure.py 100% <ø> (ø) ⬆️
datalad/interface/base.py 90.54% <ø> (-0.06%) ⬇️
datalad/cmdline/main.py 77.4% <ø> (-0.19%) ⬇️
datalad/core/local/tests/test_create.py 100% <ø> (ø) ⬆️
datalad/interface/common_opts.py 100% <ø> (ø) ⬆️
datalad/utils.py 87.13% <100%> (+0.07%) ⬆️
datalad/core/local/tests/test_resulthooks.py 100% <100%> (ø) ⬆️
datalad/interface/utils.py 93.69% <95.34%> (-1%) ⬇️
... and 6 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 c38dbd7...07754bd. Read the comment docs.

@mih mih changed the title Rip out porc-pre/post in order to get a straight fix for gh-3962 Refurbish eval_func() Dec 23, 2019
@mih mih added the merge-if-ok label Dec 23, 2019
@mih mih requested a review from kyleam Dec 26, 2019
@mih
Copy link
Member Author

@mih mih commented Dec 30, 2019

I'll merge this later today, unless someone stops me.

datalad/interface/utils.py Outdated Show resolved Hide resolved
datalad/interface/utils.py Outdated Show resolved Hide resolved
kyleam
kyleam approved these changes Dec 30, 2019
Copy link
Contributor

@kyleam kyleam left a comment

I'm not very familiar with this part of the code base, but the core changes here make sense as far as I can tell, though I didn't step through any of the code or try to run anything.

I struggled when reading through the commit messages and PR description for this series. They feel more like notes for you rather than explanations for reviewers or future readers/diggers.

@mih
Copy link
Member Author

@mih mih commented Dec 30, 2019

Thx @kyleam !

I have fixed the typos. Your assessment re documentation quality is, unfortunately, correct. Initially this was supposed to fix the hooks implementation, but then it got messy. :(

@kyleam
Copy link
Contributor

@kyleam kyleam commented Dec 30, 2019

@mih

I'll merge this later today, unless someone stops me.

No one has yelled "stop!" :), so I'll merge this in now. Thanks @mih.

@kyleam kyleam merged commit 07754bd into datalad:master Dec 30, 2019
14 of 17 checks passed
kyleam added a commit that referenced this issue Dec 30, 2019
@mih mih deleted the bf-hooks branch Dec 31, 2019
@mih
Copy link
Member Author

@mih mih commented Dec 31, 2019

Thx @kyleam

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants