DM-43769: Improve Pydantic support for sphgeom.Region and Timespan #997
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TallJimbo
force-pushed
the
tickets/DM-43769
branch
from
1dd159a
to
a803022
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #997 +/- ##
==========================================
- Coverage 88.98% 88.97% -0.01%
==========================================
Files 344 344
Lines 44141 44199 +58
Branches 9084 9100 +16
==========================================
+ Hits 39278 39326 +48
- Misses 3549 3556 +7
- Partials 1314 1317 +3 ☔ View full report in Codecov by Sentry. |
TallJimbo
force-pushed
the
tickets/DM-43769
branch
2 times, most recently
from
c1cfd21
to
ce88844
Compare
dhirving
approved these changes
Apr 12, 2024
| adapter.validate_json({}) | ||
| with self.assertRaises(ValueError): | ||
| adapter.validate_json((b"this is not a region").hex()) | ||
|
|
There was a problem hiding this comment.
Might be worth adding a test for "a string, but not a hex string." (Pretty sure it will pass already.)
|
Also in the future feel free to delegate any little tickets like this to me, I'm sure you have much bigger fish to fry :D |
Pydantic's symbols (especially Field) often aren't obviously from Pydantic on their own, so I think it's one of the cases where 'import pydantic' is better than 'from pydantic import <stuff>', especially when '<stuff>' is long, as it is about to become here.
This cleaned up some special-casing in DimensionRecord serialization, but it required some other special-casing to be replaced, for two primary reasons: - The 'direct' deserialization methods (which we *may* not even need) is hashing entire proto-records in its cache system, while it should just be using required-dimension-value tuples. - I haven't tried to replace the special-casing for 'region' and 'hash' (this one really bakes in some bad assumptions about the concrete universe!). I have thoughts for addressing both issues but I think they're more than I want to do on this branch.
This requires making Timespan.nsec public, which is only slightly unfortunate; we can declare it frozen to maintain immutability, and we couldn't realistically change the implementation in the future given that we've already exposed that in how it's serialized. This obsoletes the previous commit on this branch that used pydantic_core, but I'm keeping that version in the commit history in case it's meaningfully more efficient in validation or memory use (I suspect it's slightly more efficient, but not enough to matter).
Co-authored-by: David H. Irving <david.irving@noirlab.edu>
Co-authored-by: David H. Irving <david.irving@noirlab.edu>
TallJimbo
force-pushed
the
tickets/DM-43769
branch
from
d9e12e9
to
ad25039
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
doc/changesconfigs/old_dimensions