Degrade to identity on a malformed transform substring instead of raising ValueError - #247
Degrade to identity on a malformed transform substring instead of raising ValueError#247eeshsaxena wants to merge 1 commit into
Conversation
_parse_transform_substr raised a bare ValueError when a transform substring had
non-numeric values (float('x')) or the wrong number of parentheses (the
type(...) split). parse_transform already warns and returns the identity matrix
for an unknown transform type or a wrong argument count, so handle these the
same way instead of raising.
|
Hi! Gentle nudge on this one whenever you have some bandwidth. It's a small, self-contained fix ( |
|
I have one suggestion, otherwise LGTM. What do you think @eeshsaxena? |
|
Thanks! I think the suggestion didn't actually come through on my end though, I'm not seeing an inline comment or a review body on the PR, just the "one suggestion" note. Mind re-posting it? Happy to make the change once I can see what you had in mind. |
|
|
||
| transform = np.identity(3) | ||
| try: | ||
| values = list(map(float, filter(None, value_str.split(' ')))) |
There was a problem hiding this comment.
| values = list(map(float, filter(None, value_str.split(' ')))) | |
| values = [float(s) for s in value_str.split()] |
A bit easier to read (IMO) and robust against other/multiple whitespace chars.
My bad, I never pushed submit 🙃 |
|
|
I see, this is really the design here. Your change makes total sense. |
parse_transform's error policy for invalid syntax was mixed, by accident rather than design: unknown transform types and wrong argument counts warned and degraded to identity, while non-numeric values and malformed parentheses raised bare errors from float() and tuple unpacking, and anything after the last ')' was silently discarded. By default all invalid substrings now warn and contribute an identity matrix, with valid substrings still applied -- the behavior proposed in PR #247, and no change for input that already parsed. For callers who prefer errors, parse_transform(s, strict=True) raises a ValueError whose message identifies the offending substring. Also split values on any whitespace (tabs, newlines) rather than only spaces, as the SVG spec allows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
parse_transform's error policy for invalid syntax was mixed, by accident rather than design: unknown transform types and wrong argument counts warned and degraded to identity, while non-numeric values and malformed parentheses raised bare errors from float() and tuple unpacking, and anything after the last ')' was silently discarded. By default all invalid substrings now warn and contribute an identity matrix, with valid substrings still applied -- the behavior proposed in PR #247, and no change for input that already parsed. For callers who prefer errors, parse_transform(s, strict=True) raises a ValueError whose message identifies the offending substring. Also split values on any whitespace (tabs, newlines) rather than only spaces, as the SVG spec allows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
I've merged this in with #250 . Thanks for the fixes @eeshsaxena ! |
2 participants
parse_transformalready tolerates malformed input in most places: it warns and returns the identity matrix for an unknown transform type or a wrong number of arguments. But two cases still raise a bareValueError:parse_transform('matrix(1 x 3 4 5 6)')->float('x')raises;(, e.g. a stray'matrix', so thetype, values = substr.split('(')unpack fails.I guarded both so they warn and fall back to the identity matrix, matching the existing
_check_num_parsed_values/ unknown-type behavior. Valid transforms are unchanged.Added a test with the malformed variants; it raises ValueError on master and passes with the change, and the rest of the parsing tests still pass.