Add additional tests for unexpected fields in units#183
Conversation
| "chain", # Survey units eliminated | ||
| "SECONDS" # Not just case insensitivity | ||
| "SECONDS", # Not just case insensitivity | ||
| "lb : in^3", # Not just case insensitivity |
There was a problem hiding this comment.
Good eyes, copypasta
| if next(token for token in tokens).type == NUMBER: | ||
| return input_string # The unit can't have a leading number; scaling factors are internal |
There was a problem hiding this comment.
Why does returning the input string lead to the desired error being thrown?
Also, why is a leading number not allowed? If g / 100 L is then why not 100 L / g? Or is this to cover for the cases in which the input string is just a number?
There was a problem hiding this comment.
We exit the preprocessor without mangling the leading digit, so it's handled as it would have been handled by default. We could probably get equivalent functionality by just swallowing the first term, but I didn't want to have to deal with possible weirdness following from that if that first term would have set some of the internal booleans.
No leading digit because normal Pint behavior says that should be a magnitude on a value, and that kinda matches. I personally also can't think of a customary unit that has a scaling factor that has it up front like that. It conveniently also covers the case where it's just a number.
There was a problem hiding this comment.
so it's handled as it would have been handled by default
I don't know what that is
There was a problem hiding this comment.
Not allowing leading numbers is reasonable. And if this the effect of producing the desired error message then that's fine. I just find the logic to be unclear
There was a problem hiding this comment.
The preprocessor is run internally to Pint's logic, so it already has opinions (depending on how one invokes the parse) how to handle numbers.
- This preprocessor makes it so if there is a number not in the leading position, it is turned into a conversion factor token instead of lumping it into the magnitude. Without it, the units on
g / 2.5 cmare reported asg / cmor raises an error depending on if you useparse_unitsorconvert. - The gemd.units module then expresses the opinion that gemd-friendly units strings didn't have a scaling factor.
Updating the code so it just ignores the first term, which would I think be more clear.
| exponent = token.type == OP and token.string in {"^", "**"} | ||
| division = token.type == OP and token.string in {"/", "//"} | ||
| if token.type == OP: | ||
| if token.string not in {"+", "-", "*", "/", "//", "^", "**", "(", ")"}: |
There was a problem hiding this comment.
This would be more readable if the set of all allowed tokens was broken out as a constant
There was a problem hiding this comment.
Fair. I'd wanted to import it from Pint, but it's a private variable.
3 participants
Field testing showed some bad behavior:
"and:were being swallowed, leading to things that weren't valid units being accepted