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.

Sign up for GitHub

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

Jump to bottom

Support unsetting config via datalad -c :<name> #6864

Merged
merged 5 commits into from
Jul 21, 2022
Merged

Support unsetting config via datalad -c :<name> #6864

merged 5 commits into from
Jul 21, 2022

Conversation

mih
Copy link
Member

@mih mih commented Jul 20, 2022

As documented in #5653 (comment) this API was lacking a method to unset configuration items, while
all other API do support this feature. This changeset adds it.

Closes #6863

This PR is targeting maint. IMHO this is not a new feature, but rather a missing piece in the implementation of an existing feature. It is strictly backward compatible.

mih added 2 commits July 20, 2022 21:32
@mih
Support unsetting config via datalad -c :<name>
b56a890 
As documented in
datalad#5653 (comment)
this API was lacking a method to unset configuration items, while
all other API do support this feature. This changeset adds it.

Closes datalad#6863
@mih
Speed-up test execution
7d1e245 
Instead of running a complete `wtf` for just getting the config listed,
limit to this particular section. This cuts the runtime in half down to
3.7s for the entire file (rather than the previously annotated 11s for
this one test.
@mih mih added semver-patch Increment the patch version when merged cmd-config(urgation) labels Jul 20, 2022
@codecov
Copy link

codecov bot commented Jul 20, 2022 •
edited
Loading

Codecov Report

Merging #6864 (a556c22) into maint (49db421) will increase coverage by 0.95%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            maint    #6864      +/-   ##
==========================================
+ Coverage   90.27%   91.23%   +0.95%     
==========================================
  Files         354      354              
  Lines       46123    46127       +4     
==========================================
+ Hits        41637    42083     +446     
+ Misses       4486     4044     -442
Impacted Files Coverage Δ
datalad/cli/common_args.py 100.00% <ø> (ø)
datalad/cli/helpers.py 78.99% <100.00%> (+0.17%) ⬆️
datalad/cli/tests/test_main.py 95.23% <100.00%> (+0.08%) ⬆️
datalad/tests/utils.py 66.69% <0.00%> (+9.72%) ⬆️
datalad/tests/test_tests_utils.py 92.34% <0.00%> (+92.34%) ⬆️

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 49db421...a556c22. Read the comment docs.

yarikoptic
yarikoptic approved these changes Jul 20, 2022
View reviewed changes
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Looks LGTM but left some "stylistic" recommendations and filed related but not directly to this PR #6865

@mih @yarikoptic
Change formatting spec style
40985d4 
As suggested by @yarikoptic.

This adds `(...|...)` to ways of describing the concept of alternative values in the CLI. We already use `...|...` and `{..., ...}`. But given the absence of style guidelines, this change should be ok.

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
mih
mih commented Jul 21, 2022
View reviewed changes
@mih
Copy link
Member Author

mih commented Jul 21, 2022 •
edited
Loading

I filed #6866 to capture one outcome of this PR.

I now noticed that one particular travis run fails wrt the new test. I am not 100% certain what this test run's purpose is, but this needs to be investigated before things can go further. I will not have the resources to do this soonish (was hoping to have this be an evening side-quest). I will put the PR into draft mode for now. Fell free to close, once it is hanging here for too long. I have no real ETA

They key issue is turning on debug logging:

this passes:

DATALAD_LOG_LEVEL=info python -m pytest -s -v --pyargs datalad/cli/tests/test_main.py::test_cfg_override

this fails

DATALAD_LOG_LEVEL=debug python -m pytest -s -v --pyargs datalad/cli/tests/test_main.py::test_cfg_override

@mih mih marked this pull request as draft July 21, 2022 07:30
@mih mih marked this pull request as ready for review July 21, 2022 09:34
@mih
Copy link
Member Author

mih commented Jul 21, 2022

Works now. I remembered where to look for the solution.

@yarikoptic
Copy link
Member

note that in #6864 (comment) two invocations are the same -- I guess should've changed log level to debug in bad one.

@yarikoptic
Copy link
Member

all green, approved, let's proceed

@yarikoptic yarikoptic merged commit cefecae into datalad:maint Jul 21, 2022
@mih mih deleted the bf-6863 branch July 21, 2022 15:46
@mih
Copy link
Member Author

mih commented Jul 21, 2022

Yes, 'debug' as log level was the trigger. Adjusted the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yarikoptic yarikoptic yarikoptic approved these changes

@bpoldrack bpoldrack bpoldrack approved these changes

Assignees
No one assigned
Labels
cmd-config(urgation) semver-patch Increment the patch version when merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mih @yarikoptic @bpoldrack