fix(workflows): compare non-numeric strings lexicographically instead of returning False#3323
Merged
mnriem merged 1 commit intoJul 6, 2026
Conversation
… of returning False
_safe_compare coerced both operands to int/float unconditionally for <, >, <=,
>=. any non-numeric string (an iso date, a version tag, a name) failed that
coercion and the whole comparison silently returned False -- so
`{{ inputs.d < '2026-02-01' }}` was False even when the date was earlier.
only coerce when both operands look numeric; otherwise compare the original
values, so two strings order lexicographically the way python does and two
numeric strings still compare as numbers ("10" > "9"). a number vs a
non-numeric string stays incomparable and yields False.
add a regression test covering dates, plain strings, numeric strings, and the
number-vs-string case.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes workflow expression ordering comparisons so non-numeric strings are compared lexicographically (instead of silently returning False), while preserving numeric-string ordering as numeric comparisons. This improves correctness for common workflow conditions involving dates, tags, and names.
Changes:
- Add
_coerce_number()and update_safe_compare()to only apply numeric coercion when both operands can be treated as numbers; otherwise compare original values (withTypeErroryieldingFalse). - Add a regression test covering ISO date ordering, plain string ordering, numeric-string ordering, and number-vs-non-numeric-string behavior.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/expressions.py |
Refines _safe_compare to support lexicographic ordering for non-numeric strings while keeping numeric comparisons correct and safe. |
tests/test_workflows.py |
Adds regression coverage to prevent string ordering comparisons from silently returning False. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
Collaborator
|
Thank you! |
This file contains hidden or 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
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.
fixes #3321
what
ordering comparisons (
<,>,<=,>=) between non-numeric strings in workflow expressions silently returnedFalse._safe_comparecoerced both operands to int/float unconditionally, so any non-numeric string (iso date, version tag, name) failed coercion and the whole comparison returnedFalse:how
coerce to a number only when both operands look numeric; otherwise compare the original values. so:
"2026-01-01" < "2026-02-01"is True)"10" > "9"is True, not lexical False)==/!=are untouched (they never went through_safe_compare).tests
added
test_ordering_comparison_of_non_numeric_strings(dates, plain strings, numeric strings, number-vs-string). verified it fails on the current code (returns False) and passes with the fix; fullTestExpressionspasses.