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

Fixup attrs #40

Merged
merged 4 commits into from
Aug 23, 2024
Merged

Conversation

ghidalgo3
Copy link
Collaborator

Addresses #30 , xarray tends to put numpy arrays in attrs which are not immediately JSON serializable.

@TomAugspurger
Copy link
Collaborator

Thanks, would you mind adding a simple test for this so we don't regress on it in the future?

Also, I'll make you a maintainer here. But I apparently don't have admin so I'll need an owner on stac-utils to make me an admin. Maybe @lossyrob could help with that? I think https://github.com/stac-utils/xstac/settings is the URL.

@lossyrob
Copy link
Member

Added both of you as admins.

@ghidalgo3
Copy link
Collaborator Author

Thanks, Tom and Rob! Added a unit test that reprod the issue.

Copy link
Collaborator

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Nice teamwork :)

Comment on lines 172 to 176
try:
import json
json.dumps(result.to_dict())
except Exception:
pytest.fail("Failed to serialize to JSON")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move the JSON import to the top and then just do json.dumps(result.to_dict()) in the test? No need to catch the exception. An unhandled one will cause the test to fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@TomAugspurger
Copy link
Collaborator

Looks like a CI failure on linting: https://github.com/stac-utils/xstac/actions/runs/10527664242/job/29173634803?pr=40#step:6:23.

Might want to do a pre-commit install to get these hooks locally.

@TomAugspurger TomAugspurger merged commit c9e666f into stac-utils:main Aug 23, 2024
1 check passed
@TomAugspurger
Copy link
Collaborator

Thanks! LMK if a release would be helpful, it should just require a new release on GitHub and PyPI will be automated from there.

@ghidalgo3 ghidalgo3 deleted the guhidalgo/handlenumpyattrs branch September 6, 2024 20:37
@ghidalgo3
Copy link
Collaborator Author

I will probably get a few more PRs into xstac before I ask for a release.

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.

3 participants