-
Notifications
You must be signed in to change notification settings - Fork 122
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
Make table extension columns serializable #992
Conversation
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.
Agreed that the new functionality in the base class is useful; previous fixes (e.g.
pystac/pystac/extensions/raster.py
Lines 693 to 697 in 7819728
@bands.setter | |
def bands(self, v: Optional[List[RasterBand]]) -> None: | |
self._set_property( | |
BANDS_PROP, map_opt(lambda bands: [b.to_dict() for b in bands], v) | |
) |
_set_property
magic to use this new base-class functionality.
Codecov ReportBase: 90.06% // Head: 90.13% // Increases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #992 +/- ##
==========================================
+ Coverage 90.06% 90.13% +0.06%
==========================================
Files 47 47
Lines 6161 6164 +3
Branches 917 920 +3
==========================================
+ Hits 5549 5556 +7
+ Misses 431 426 -5
- Partials 181 182 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
c7dde3c
to
e306f1c
Compare
Related Issue(s): #872
Description:
The table extension was not serializable because the list of columns was not getting serialized properly. This PR takes inspiration from the original recommendation (thanks @martinfleis!) but pushes the change further back into the mixin class.
Note:
The
_set_property
functionality seems pretty useful overall and I was wondering if it should be use more thoroughly or if there is a reason not to use it.PR Checklist:
pre-commit run --all-files
)scripts/test
)