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

status: Provide special treatment of "this dataset" path #5663

Merged
merged 2 commits into from May 18, 2021

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented May 13, 2021

When called with a path argument that points to a dataset and that
dataset has a parent dataset, status() checks if the original path
ends with "/". No trailing separate indicates that the dataset as a
whole is of interest, leading to a query in the parent for that
dataset.

The above logic is confusing when dataset and path point to the
same dataset. status() with a parent-less dataset works (and behaves
as the path argument ended with "/"), while a nested dataset fails
with

[ERROR ] dataset containing given paths is not underneath the
reference dataset [...]

3dea65a (BF: status: Provide special treatment of "." path,
2019-04-12) avoided the above error for the path="." case. However,
the same error can be triggered by calling status() with a path
argument other than "." that points to the same location as the
dataset argument.

Adjust the condition from 3dea65a to handle that too.

Closes #5657.


cc: @mih

@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

Merging #5663 (5aac1d9) into maint (2c60c53) will decrease coverage by 5.46%.
The diff coverage is 100.00%.

❗ Current head 5aac1d9 differs from pull request most recent head 3c68793. Consider uploading reports for the commit 3c68793 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5663      +/-   ##
==========================================
- Coverage   90.31%   84.84%   -5.47%     
==========================================
  Files         299      296       -3     
  Lines       42312    42298      -14     
==========================================
- Hits        38214    35888    -2326     
- Misses       4098     6410    +2312     
Impacted Files Coverage Δ
datalad/core/local/status.py 96.42% <100.00%> (+0.03%) ⬆️
datalad/core/local/tests/test_status.py 98.21% <100.00%> (+0.04%) ⬆️
datalad/support/tests/utils.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/metadata/extractors/tests/test_audio.py 19.35% <0.00%> (-80.65%) ⬇️
datalad/metadata/extractors/xmp.py 12.96% <0.00%> (-79.63%) ⬇️
datalad/metadata/extractors/exif.py 18.75% <0.00%> (-78.13%) ⬇️
datalad/metadata/extractors/image.py 19.35% <0.00%> (-77.42%) ⬇️
datalad/metadata/extractors/audio.py 20.00% <0.00%> (-77.15%) ⬇️
datalad/metadata/extractors/tests/test_exif.py 24.00% <0.00%> (-76.00%) ⬇️
datalad/metadata/extractors/tests/test_xmp.py 26.92% <0.00%> (-73.08%) ⬇️
... and 88 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 2c60c53...3c68793. Read the comment docs.

status='error',
message='path not underneath this dataset',
logger=lgr)
continue
else:
if dataset and root == str(p) and \
not (orig_path.endswith(op.sep) or
orig_path == "."):
root == ds_path):
Copy link
Member

Choose a reason for hiding this comment

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

and no op.normpath or alike needed, and something like ./. or subdir/.. etc would work too? would be nice to add to the tests then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. At the moment, it depends on whether it's something resolve_path adjusts (and, as its docstring notes, that avoids some normalization that can be problematic for the reasons discussed in pathlib's documentation).

Given the comparison is with Dataset().path, I think the best approach is to just feed root through Dataset(). That way there should be no need to match logic or keep it in sync.

Will push an update to condition/tests.

When called with a path argument that points to a dataset and that
dataset has a parent dataset, status() checks if the original path
ends with "/".  No trailing separate indicates that the dataset as a
whole is of interest, leading to a query in the parent for that
dataset.

The above logic is confusing when `dataset` and `path` point to the
same dataset.  status() with a parent-less dataset works (and behaves
as the path argument ended with "/"), while a nested dataset fails
with

    [ERROR ] dataset containing given paths is not underneath the
    reference dataset [...]

3dea65a (BF: status: Provide special treatment of "." path,
2019-04-12) avoided the above error for the path="." case.  However,
the same error can be triggered by calling status() with a path
argument other than "." that points to the same location as the
dataset argument.

Adjust the condition from 3dea65a to handle that too.

Closes datalad#5657.
@kyleam kyleam merged commit e8ea043 into datalad:maint May 18, 2021
@kyleam kyleam deleted the status-ds-equal-path branch May 18, 2021 13:30
@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Jun 2, 2021
@yarikoptic yarikoptic mentioned this pull request Jun 3, 2021
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants