feat: ✨ add path_*() functions (with their verify_*()'ers)
#730
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
The utility functions expands on the raised error to include existing ID's.
The utility functions make it easier to create the folder structure of packages and resources for testing the `path_()*` functions.
Since they are now in the init file.
signekb
changed the title
path_*() functions with utilspath_*() functions with helper functions
signekb
commented
Sep 25, 2024
martonvago
requested changes
Sep 26, 2024
Instead of converting it to a string and then back to Path in the tests
lwjohnst86
previously approved these changes
Sep 30, 2024
There was a problem hiding this comment.
@martonvago and @signekb I refactored the verify function into two, because the original one was confusing to me and had too many arguments for the purpose I interpreted it had. Makes the code a bit cleaner.
sprout/core/verify_is_package_dir.py
Outdated
There was a problem hiding this comment.
I split the original verify function into two rather than one, and simplified them a lot. The original confused me.
lwjohnst86
changed the title
path_*() functions with helper functionspath_*() functions (with their verify_*()'ers)
martonvago
requested changes
Oct 1, 2024
| A Path to the resources within the package. | ||
| """ | ||
| path = path_package(package_id) / "resources" | ||
| verify_is_package_dir(path.parent) |
There was a problem hiding this comment.
Doesn't path_package already do this (meaning this will never throw)?
lwjohnst86
previously approved these changes
Oct 2, 2024
There was a problem hiding this comment.
@martonvago since the verify_is_file() already has a unit test for it, I'm not too worried that we don't test that the files exist or not for these tests.
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.
Description
I decided to have a script for path functions related to packages and one related to resources. I found that it became too long - for my taste at least - with the docstrings otherwise.
To add existing ID's to the raised errors, I have added two verify functions.
To test the path functions, I have created some helper functions to create the directory structure of packages and resources.
Closes #714
Reviewer Focus
This PR needs an in-depth review.
Checklist