Unconditionally store image path in POSIX notation in configuration #243
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
This is a plain code move from `containers_list` into a standalone function. Added value is - reusability - documentation, incl. type annotation Also adds rendering of the utility module's documentation. This is a first step towards resolving datalad#238
This is a common use case in commands other than `containers_list`. Albeit not strictly necessary (all configuration could be read and processed first, and sub-selection could happen in user code), it is a low-complexity change. Moreover, we envision more configuration post-processing to happen in the future (see datalad#238). This would change the cost assessment of loading everything upfront.
This changeset maintains the exact behavior of the command, and merely adds documentation and replaces custom configuration access with the new `get_container_configuration()` helper. The documentation may communicate suboptimal command semantics, but these were given before, and are only written down now. Also see datalad#240 for additional aspects re a future refactoring. Configuration writes now use the ``scope=branch`` approach. It was introduced with datalad v0.16 and requires no dependency adjustments.
...for the purpose of a standardization of container configuration reads on `get_container_configuration()`.
This is done by turning of the result rendering. This revealed that the command implementations cause uncondtional result rendering datalad#241
This is the last change for establishing `get_container_configuration()` as the single code piece for reading container documentation. This opens the door for implementing on-read normalization of configuration items, such as the image path. Closes datalad#238
|
Code Climate has analyzed commit cd49d66 and detected 0 issues on this pull request. View more on Code Climate. |
Possible a little less complex to understand.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #243 +/- ##
==========================================
+ Coverage 94.69% 94.70% +0.01%
==========================================
Files 25 25
Lines 1093 1133 +40
==========================================
+ Hits 1035 1073 +38
- Misses 58 60 +2
☔ View full report in Codecov by Sentry. |
This establishes a fixed format for the configuration. Previously, the path was stored in platform conventions without any indication of the nature of that platform. This turned platform detection into guesswork. Some of that guesswork is now implemented in a path normalization helper (`_normalize_image_path()`). it is executed on-read, in order to keep older dataset configuration functional for the cases - we can tell the platform conventions, because the image is placed under `.datalad\\environments\\` (the default) - we can locate a matching element on the file system Notably, this guesswork does not cover the case of an image that was configured on windows, and is hosted in a currently uninstalled subdataset. Closes datalad#224
yarikoptic
reviewed
Oct 12, 2023
yarikoptic
reviewed
Oct 12, 2023
yarikoptic
reviewed
Oct 12, 2023
yarikoptic
reviewed
Oct 12, 2023
yarikoptic
approved these changes
Oct 12, 2023
The faulty path delimiter is a `\\`, not just a single backlash. Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
|
Thanks for the reviews! Very helpful! |
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.
[This PR sits on #242, only the last commit is unique and relevant here]
This establishes a fixed format for the configuration. Previously,
the path was stored in platform conventions without any indication
of the nature of that platform. This turned platform detection into
guesswork. Configuration created on windows was not functional on other platforms.
Some of that guesswork is now implemented in a path normalization
helper (
_normalize_image_path()). It is executed on-read, in orderto keep older dataset configuration functional for the cases
under
.datalad\\environments\\(the default)Notably, this guesswork does not cover the case of an image that
was configured on windows, and is hosted in a currently uninstalled
subdataset. This seems rare enough of a case to not deserve the implied complexity of supporting it in a backward compatible way.
Closes #224
TODO:
The missing lines in the coverage are
I am not planning on adding a test for this case (a path that is verified to be in windows convention by finding a matching filesystem item). If this is unacceptable, I am OK with removing support for this entirely.