Skip to content
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 data to dataset via object with read() method #540

Merged
merged 7 commits into from
Mar 12, 2024

Conversation

aaraney
Copy link
Member

@aaraney aaraney commented Mar 11, 2024

Enable adding data via add_data() to existing dataset using an object with a read() -> bytes method. The data argument now accepts either a filename or something that has a read() -> bytes method. Implementation added for ObjectStoreDatasetManager.

closes #319

Additions

  • DatasetManager.add_data()'s data argument now accepts either a filename or something that has a read() -> bytes method. Implementation added for ObjectStoreDatasetManager.

@aaraney aaraney mentioned this pull request Mar 11, 2024
Copy link
Contributor

@robertbartel robertbartel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By and large this looks really good. I had a couple of comments on the changes themselves, but very small things. We should bump the dataservice dependency versions for these packages, though, to make sure they get used. Also, I haven't looked at why yet, but there are some test failures.

@@ -546,8 +548,8 @@ def add_data(self, dataset_name: str, dest: str, data: Optional[bytes] = None, s
A path-like string specifying a location within the dataset (e.g., file, object, sub-URL) where the data
should be added.
data : Optional[bytes]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor, but need to tweak the type for data in the docstring.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, great catch! I missed this one! Just fixed it.

@aaraney aaraney force-pushed the osm-accepts-data-via-reader branch from 615dbc3 to 7075f3b Compare March 12, 2024 15:10
@aaraney
Copy link
Member Author

aaraney commented Mar 12, 2024

Just rebased after #542 was merged. Lets see if the IT tests pass now.

@aaraney aaraney force-pushed the osm-accepts-data-via-reader branch from 7075f3b to 51275ff Compare March 12, 2024 15:15
@aaraney
Copy link
Member Author

aaraney commented Mar 12, 2024

Had the wrong logic in the test 😂, so it failed, correctly. Just fixed it and repushed.

@aaraney
Copy link
Member Author

aaraney commented Mar 12, 2024

Sweet, looks like the tests are all passing now.

@robertbartel robertbartel merged commit c07fcc1 into NOAA-OWP:master Mar 12, 2024
8 checks passed
@aaraney
Copy link
Member Author

aaraney commented Mar 12, 2024

Thanks, @robertbartel!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add abstract add_item and add_items methods to DatasetManager
2 participants