Make TEDAPIApiVersion comparable.#363
Conversation
Give it real comparison operators ordered by the date the label encodes and do not rely on the side-effect that strings sort the way we want. - Dates without a day of the month default to "0" -- So V2024_06 would come before V2024_06_01 - The VYYYY_MM[_DD] format is now enforced, not conventional: non-conforming - A non-version operand raises TypeError rather than returning NotImplemented. - Adds 22 tests covering ordering, the optional day, format rejection.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #363 +/- ##
==========================================
+ Coverage 39.55% 40.26% +0.71%
==========================================
Files 65 65
Lines 10657 10785 +128
Branches 1324 1334 +10
==========================================
+ Hits 4215 4343 +128
Misses 6187 6187
Partials 255 255
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jasonacox-sam
left a comment
jasonacox-sam
left a comment
There was a problem hiding this comment.
The implementation is solid. A few things I specifically looked at:
Comparison semantics via label parsing, not declaration order — correct call. Using the date encoded in VYYYY_MM[_DD] means a member inserted out of sequence still sorts right, and it makes the label format an enforced contract rather than a convention.
_rank_of raises TypeError instead of returning NotImplemented — this is the right design for a str subclass. Returning NotImplemented would silently fall back to str's lexical comparison (e.g. 'v2026_06' > 'V2026_06' in ASCII = True, quietly reversing the order). Raising loudly is the correct choice here, and the comment explains it well.
_validate_member_labels() called at import — good defensive programming. If a future developer adds V2027_01 as a typo like V2027_1, it fails at import and points straight at the offender instead of surfacing as a broken comparison hours later at some call site.
_NO_DAY = 0 for month-only labels — the rationale holds. Month-only labels came first; dated labels in the same month come later. Day 0 can't collide with a real date because _parse_label rejects days outside 01–31.
One minor thing: the comment at the top of the test section says "Ordering is by declaration order, not by comparing the string labels" — I think this is trying to say the opposite (the ordering uses the date in the label, not raw string comparison), but the phrasing reads as though it's describing the implementation rather than the alternative it avoids. Worth a quick clarification when you get a chance, but it's a doc nit and doesn't affect correctness.
Test coverage looks comprehensive — members, reflexivity, threshold semantics, plain string operands, and reflected ops. The reflected-op tests in particular are the ones that would catch a regression if this ever stopped being a str subclass.
Approved. Once #363 and #364 are in, #359's diff should be materially simpler.
— Sam ⚡
3 participants
Summary
In the spirit of small, modular, atomic PRs rather than huge ones, I'm trying to find pieces of The commit to add "bearer" mode that I can part out.
The bearer PR is the first time the V2026_06 API mechanisms are actually used. Thus, parts of the code that rely on it need to have a (pseudocode): "if api_version is >= V2026_06" check.
Well, it would be nice if we had a clean way to compare API versions. This is that, and it stands by itself.
Testing is primarily unit tests. The actual function of the enumeration isn't touched.