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

Test, documentation, fixes and public introduction of parse_gitconfig_dump() #5509

merged 6 commits into from
Mar 19, 2021


Copy link

@mih mih commented Mar 18, 2021

This PR (in order of importance):

Although this series slightly complicates the processing in this critical function, I do not observe a performance impact (see commit message).

mih added 5 commits March 18, 2021 10:59
- Remove underscore from name, keep private alias for now
- Added minimal documentation on return values
- Add test to show that it cannot handle multi-line values
Before only the first line was reported.
Do a more strict matching of git-config keys, and use it to detect
and discard non syntax compliant pieces of output while parsing.
This is able to discard any contaminating output that has a trailing
newline, and does not (accidentally) fully consists of a valid
git-config key.

Tested on an "average" dataset with a "normal" amount of configuration
there does not seem to be a negative performance impact.

from datalad.config import ConfigManager
from datalad.api import Dataset
timeit ConfigManager(dataset=ds)

- Before: 12.1 ms ± 93.2 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
- After: 12 ms ± 112 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

Fixes dataladgh-5502
Old name stays an place as an alias for now.
Copy link

codecov bot commented Mar 18, 2021

Codecov Report

Merging #5509 (6a8efb3) into maint (d429a90) will decrease coverage by 0.39%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5509      +/-   ##
- Coverage   90.16%   89.76%   -0.40%     
  Files         296      296              
  Lines       41868    41893      +25     
- Hits        37750    37605     -145     
- Misses       4118     4288     +170     
Impacted Files Coverage Δ
datalad/ 96.64% <100.00%> (+0.11%) ⬆️
datalad/support/ 91.82% <100.00%> (ø)
datalad/tests/ 100.00% <100.00%> (ø)
datalad/ui/ 56.25% <0.00%> (-21.88%) ⬇️
datalad/support/ 29.87% <0.00%> (-10.39%) ⬇️
datalad/core/local/tests/ 92.85% <0.00%> (-7.15%) ⬇️
datalad/support/tests/ 92.50% <0.00%> (-5.00%) ⬇️
datalad/tests/ 91.15% <0.00%> (-4.09%) ⬇️
datalad/customremotes/tests/ 96.87% <0.00%> (-3.13%) ⬇️
datalad/ 82.09% <0.00%> (-3.00%) ⬇️
... and 35 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 d429a90...6a8efb3. Read the comment docs.

Copy link

@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.

LGTM. Left a minor wish.

datalad/ Outdated Show resolved Hide resolved
datalad/ Outdated Show resolved Hide resolved
datalad/ Show resolved Hide resolved
@mih mih merged commit 789a5a5 into datalad:maint Mar 19, 2021
@mih mih deleted the bf-gitcfg branch March 19, 2021 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants