Change semantics of PathBasedItem names
#583
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, the `name` attribute was some kind of `PurePath` instance. The precise nature varied across item types and corresponding iterators. This led to two problems: - Performance (see datalad#581): the creation of a Path instance is not particularly cheap. If a consumer does not need such an instance, and unconditional creation is a waste of resources - Abstraction (see datalad#430): the Path representation of a path-like name does not always match the exact semantics of the original name (it might have a trailing slash that has a purpose, etc) Both problems are now addressed by relaxing the type of `.name` of a `PathBasedItem`. It can now be anything. Consumers that require an actual Path instance can use the `.path` attribute. An analog access is also implemented for `.link_target` (which now remains literal), that is accompanied by `.link_target_path`, where it was necessary. This change in approach removed the need for the `_ZipFileDirPath` workaround that was used to re-establish compatibility of a Path-based `.name` with the conventions in a `ZipFile` collection/container. With the `.name` attribute retaining its native semantics, the workaround is no longer needed as was removed. In order to document the (lack of) implemented homogeneous conventions for path-based items, the docstrings of all iterators have been amended with information on the nature of the `.name` attribute yielded by them. The corresponding data classes have received docstrings for the newly added properties for Path access. These properties uniformly use the `cached_property` decorator. For the lifetime of an item, the nature of such a path should never change, and caching it automatically addresses the significant creation cost on accessing a path representation repeatedly. Closes datalad#554 Closes datalad#581
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #583 +/- ##
==========================================
- Coverage 92.45% 92.43% -0.02%
==========================================
Files 149 149
Lines 10775 10779 +4
Branches 1618 1617 -1
==========================================
+ Hits 9962 9964 +2
- Misses 636 638 +2
Partials 177 177 ☔ View full report in Codecov by Sentry. |
|
Ok, let's go without review |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the
nameattribute was some kind ofPurePathinstance. The precise nature varied across item types and corresponding iterators.This led to two problems:
PathBasedItem#581): the creation of a Path instance is not particularly cheap. If a consumer does not need such an instance, and unconditional creation is a waste of resourcesBoth problems are now addressed by relaxing the type of
.nameof aPathBasedItem. It can now be anything. Consumers that require an actual Path instance can use the.pathattribute. An analog access is also implemented for.link_target(which now remains literal), that is accompanied by.link_target_path, where it was necessary.This change in approach removed the need for the
_ZipFileDirPathworkaround that was used to re-establish compatibility of a Path-based.namewith the conventions in aZipFilecollection/container. With the.nameattribute retaining its native semantics, the workaround is no longer needed as was removed.In order to document the (lack of) implemented homogeneous conventions for path-based items, the docstrings of all iterators have been amended with information on the nature of the
.nameattribute yielded by them. The corresponding data classes have received docstrings for the newly added properties for Path access.These properties uniformly use the
cached_propertydecorator. For the lifetime of an item, the nature of such a path should never change, and caching it automatically addresses the significant creation cost on accessing a path representation repeatedly.Closes #554
Closes #581