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

Design document on URL substitution feature #6065

Merged
merged 1 commit into from Oct 12, 2021
Merged

Conversation

mih
Copy link
Member

@mih mih commented Oct 8, 2021

Fixes #6044

@mih mih added the documentation Documentation-related issue label Oct 8, 2021
@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #6065 (b97c71c) into maint (9834d78) will decrease coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #6065      +/-   ##
==========================================
- Coverage   89.84%   89.64%   -0.20%     
==========================================
  Files         312      312              
  Lines       42189    42188       -1     
==========================================
- Hits        37904    37820      -84     
- Misses       4285     4368      +83     
Impacted Files Coverage Δ
datalad/support/due.py 48.00% <0.00%> (-32.00%) ⬇️
datalad/metadata/extractors/tests/test_base.py 87.80% <0.00%> (-12.20%) ⬇️
datalad/metadata/extractors/tests/test_xmp.py 88.46% <0.00%> (-11.54%) ⬇️
datalad/metadata/extractors/tests/test_image.py 88.46% <0.00%> (-11.54%) ⬇️
datalad/metadata/tests/test_extract_metadata.py 91.11% <0.00%> (-8.89%) ⬇️
datalad/metadata/extractors/tests/test_exif.py 92.00% <0.00%> (-8.00%) ⬇️
datalad/interface/shell_completion.py 92.59% <0.00%> (-7.41%) ⬇️
datalad/metadata/extractors/tests/test_audio.py 93.54% <0.00%> (-6.46%) ⬇️
datalad/support/tests/test_due_utils.py 78.04% <0.00%> (-4.88%) ⬇️
datalad/downloaders/providers.py 93.83% <0.00%> (-1.77%) ⬇️
... and 23 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 9834d78...b97c71c. 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.

LGTM.

Should we mention things like /// although or may be precisely because they are different in mechanics?


A particular configuration item can be defined multiple times (see examples
below) to form a substitution series. Substitutions in the same series will be
applied incrementally, in order of their definition. If the first substitution
Copy link
Member

Choose a reason for hiding this comment

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

Athough that's a more general thing, I guess it's worth pointing out the order of consideration if config items are defined across multiple config files (like ~/.gitconfig + .git/config).

❱ git config --get-all datalad.some.thing                                                                                                  129 !
user-value
repo-value

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK there cannot be multi-value config items that are aggregated across config locations.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not true:

% git config --show-origin -l | grep mike
file:/home/mih/.gitconfig       mike.this=value
file:.git/config        mike.this=some
% git config --get-all mike.this | cat
value
some

Copy link
Member Author

Choose a reason for hiding this comment

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

I filed #6068 to keep track of this. The absence of a clear documentation on the config system must not lead to a diffusion of this info all over the docs.

@mih
Copy link
Member Author

mih commented Oct 11, 2021

I am not familiar with the details of ///, but I think it is merely a placeholder for a hardcoded URL. It is neither a URL itself, nor is this mechanism technically related. I do not plan to include it.

@yarikoptic
Copy link
Member

Yes, it is a placeholder. It is indeed not a URL on its own

@mih
Copy link
Member Author

mih commented Oct 12, 2021

Thx for the feedback. I think this is done, and open issues need to be addressed outside this file.

@mih mih merged commit 9a6d5de into datalad:maint Oct 12, 2021
@mih mih deleted the doc-urlsubst branch October 12, 2021 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation-related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants