-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: ✨ add create_package_structure()
#754
base: main
Are you sure you want to change the base?
Conversation
first version of both function and test
## Description Closes #638 ## Reviewer Focus <!-- Select quick/in-depth as necessary --> This PR needs a quick review. It's pretty simple :) --------- Co-authored-by: Kris Beicher <[email protected]>
## Description This renames `create_properties_object()` to `create_default_package_properties()` because it seemed more descriptive. ## Reviewer Focus <!-- Select quick/in-depth as necessary --> This PR needs a quick review. ## Checklist - [x] Added or updated tests - [x] Tests passed locally - [x] Linted and formatted code - [x] Build passed locally - [x] Updated documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awe, fills me with warm fuzzies seeing this come together!!! 🤩
create_package_structure
create_package_structure()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how short they get now that we have all the underlying functions sorted out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's super nice to see it all come together like this 🤩
I have some very minor suggestions - otherwise this looks great to me!
readme_path = create_readme_path(package_path) | ||
properties_path = create_properties_path(package_path) | ||
properties = create_default_package_properties() | ||
readme = create_readme_text(properties) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nitpicking, but I think the order of these would be nicer if they were:
readme_path = create_readme_path(package_path) | |
properties_path = create_properties_path(package_path) | |
properties = create_default_package_properties() | |
readme = create_readme_text(properties) | |
properties = create_default_package_properties() | |
properties_path = create_properties_path(package_path) | |
readme = create_readme_text(properties) | |
readme_path = create_readme_path(package_path) |
So they are "grouped" in the sense that first properties
are handled, then the README. It also matches the order that they are used in the return statement :)
assert readme_path.read_text() | ||
|
||
|
||
def test_throws_if_directory_does_not_exist(tmp_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_throws_if_directory_does_not_exist(tmp_path): | |
def test_throws_error_if_directory_does_not_exist(tmp_path): |
? :)
create_package_structure(tmp_path / "nonexistent") | ||
|
||
|
||
def test_throws_if_path_points_to_file(tmp_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_throws_if_path_points_to_file(tmp_path): | |
def test_throws_error_if_path_points_to_file(tmp_path): |
to provide the correct path location. | ||
Returns: | ||
A list of paths to the created files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A list of paths to the created files. | |
A list of paths to the two created files: The datapackage.json and the README.md. |
I think it would be nice to be more specific here :)
Description
This adds the
create_package_structure()
function.Closes #619
Reviewer Focus
This PR needs an in-depth review.
Checklist