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

DOC: build_doc: Document eval_params/_params_ override behavior #4482

Merged
merged 1 commit into from May 19, 2020

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented May 4, 2020

containers-run builds ContainersRun._params_ from Run._params_ to
extend run's interface with container-specific parameters.  In doing
so, it (presumably unintentionally) populates ContainersRun._params_
with parameters from eval_params because build_doc() modifies
Run._params_ in place.

Having eval_param keys in ContainersRun._params_ doesn't currently
cause any issues because build_doc() does a blanket update of _params_
with eval_params, but a proposed change to build_doc() in gh-4480
shows how this can fail.

Explicitly document this behavior to make it clear to callers that
they can rely on it and to prevent future build_doc() tweakers from
unknowingly changing it.

Supersedes datalad/datalad-container#119.


This will likely create a conflict with whatever eventually lands from gh-4480, but assuming that those changes keep the behavior documented in this PR, the conflict resolution will be trivial.

containers-run builds ContainersRun._params_ from Run._params_ to
extend run's interface with container-specific parameters.  In doing
so, it (presumably unintentionally) populates ContainersRun._params_
with parameters from eval_params because build_doc() modifies
Run._params_ in place.

Having eval_param keys in ContainersRun._params_ doesn't currently
cause any issues because build_doc() does a blanket update of _params_
with eval_params, but a proposed change to build_doc() in dataladgh-4480
shows how this can fail.

Explicitly document this behavior to make it clear to callers that
they can rely on it and to prevent future build_doc() tweakers from
unknowingly changing it.

Supersedes datalad/datalad-container#119.
@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #4482      +/-   ##
==========================================
+ Coverage   89.63%   89.68%   +0.04%     
==========================================
  Files         275      275              
  Lines       37103    37103              
==========================================
+ Hits        33259    33275      +16     
+ Misses       3844     3828      -16     
Impacted Files Coverage Δ
datalad/interface/base.py 90.85% <ø> (ø)
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 efdd9a8...1b8e780. Read the comment docs.

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

Thx!

@kyleam
Copy link
Contributor Author

kyleam commented May 6, 2020

@mih Thanks for taking a look.

I'll wait to merge this until gh-4480 lands so that the conflict won't need to be resolved over there.

@kyleam kyleam merged commit bce4caf into datalad:maint May 19, 2020
@kyleam kyleam deleted the build-doc-clarify branch May 19, 2020 16:56
andycon added a commit to andycon/datalad that referenced this pull request Jun 18, 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.

None yet

2 participants