refactor: ♻️ switch create_package_structure() to make properties only
#1047
Conversation
…prout into refactor/structure-to-properties-creation
Co-authored-by: Signe Kirk Brødbæk <[email protected]>
There was a problem hiding this comment.
It's annoying that this is interpreted as a completely new file, when I only revised the older file. 😠
| ) | ||
| ``` | ||
| """ | ||
| default_properties = PackageProperties.default() |
There was a problem hiding this comment.
The way we have the default method now, this makes sense, but it makes me wonder if the default method is something we should keep around. If this is the only place where we want to access these default values, maybe it would make it simpler to set them on the incoming properties directly? And maybe set them only if properties doesn't already have them set, so as not to rewrite them?
There was a problem hiding this comment.
I'm not sure I understand. Could you give an example in code?
There was a problem hiding this comment.
I just meant like this
properties.id = properties.id or str(uuid4())
properties.version = properties.version or "0.1.0"
properties.created = properties.created or get_iso_timestamp()But I only thought of this because both here and for the example package properties, it felt like default was not very nice to code around, having to convert and update when all we want to do is set some attributes.
| """ | ||
| default_properties = PackageProperties.default() | ||
| properties = properties.compact_dict | ||
| properties.update(default_properties.compact_dict) |
There was a problem hiding this comment.
This rewrites any id, created or version set on properties, right? Are we sure we want that?
There was a problem hiding this comment.
I don't think anyone wants to manually create these properties, I know I wouldn't! Do you have a use case that might highlight that need?
There was a problem hiding this comment.
Maybe if the package exists already (not necessarily as a formal data package) and has a version or an earlier created date? Of course they can still edit all that if they don't want to use the default. But to keep any explicitly set values, we would just need to switch it around to default_properties.update(properties) -- and then there are no "silent" rewrites.
| # The path should already exist as a directory. | ||
| check_is_dir(path) |
There was a problem hiding this comment.
| # The path should already exist as a directory. | |
| check_is_dir(path) | |
| # The path should already exist as a directory. | |
| check_is_dir(path) |
| Defaults to the working directory. | ||
|
|
||
| Returns: | ||
| A path to the created folder and `datapackage.json` file. |
There was a problem hiding this comment.
| A path to the created folder and `datapackage.json` file. | |
| The path to the `datapackage.json` file in the created folder. |
To make it clear that it returns one path
| to edit the `datapackage.json` properties, use this function to ensure the | ||
| properties are correctly structured before it's written. It only edits the |
There was a problem hiding this comment.
| to edit the `datapackage.json` properties, use this function to ensure the | |
| properties are correctly structured before it's written. It only edits the | |
| to edit the `datapackage.json` file, use this function to ensure the | |
| properties are correctly structured before they're written. It only edits the |
| Use this any time you want to edit the package's properties. When you need | ||
| to edit the `datapackage.json` properties, use this function to ensure the | ||
| properties are correctly structured before it's written. It only edits the | ||
| properties of the package itself, not on the data resources contained within |
There was a problem hiding this comment.
| properties of the package itself, not on the data resources contained within | |
| properties of the package itself, not of the data resources contained within |
There was a problem hiding this comment.
Maybe add a test around the behaviour of default attributes, what rewrites what
Description
The
create_package_structure()was no longer relevant, so I refactored it to thecreate_package_properties(). This function takes properties and a path as input and creates thedatapackage.jsonfile as well as the folder for the data package.I had to comment out several docstring examples since they depended on the global structure rather than the "local-first" approach.
Closes #1033
Closes #958
This PR needs an in-depth review.
Checklist
just run-all