Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Edge case for "none" unit conversions #567

Closed
peverwhee opened this issue May 23, 2024 · 1 comment
Closed

Edge case for "none" unit conversions #567

peverwhee opened this issue May 23, 2024 · 1 comment
Labels
bug For issues describing bugs, or PRs fixing bugs

Comments

@peverwhee
Copy link
Collaborator

Description

If a unit conversion involves "none", we get an error because "none" is a python keyword.

Steps to Reproduce

Using head of main, change the units for one instance of errmsg in one of the tests from "none" to "None"

Additional Context

We should have logic that can handle this, meaning:

  1. For "none" unit only, convert to lower case before comparing (to handle case differences)
  2. An exception to the "no python keywords allowed" rule for "none"? - so that if a user tries to convert "1" to "none" or something, they'll get a more helpful error.

Output

Error:

Invalid units entry, 'None', Python identifier, at...

Source of error: scripts/var_props.py", line 1172, in _get_unit_convstrs
@peverwhee peverwhee added the bug For issues describing bugs, or PRs fixing bugs label May 23, 2024
@peverwhee peverwhee mentioned this issue May 26, 2024
Closed
@peverwhee peverwhee mentioned this issue Aug 5, 2024
Merged
peverwhee added a commit that referenced this issue Aug 7, 2024
@peverwhee
Fix for "none" units edge case (#583)
58308e4 
Tiny bugfix for unit conversion edge case that was trying to convert
"None" to "none"

## Description
Convert "none" units to lowercase before comparing.

User interface changes?: No

Fixes: 
closes #567

Testing:
- Updated var_compatability_test to include a "None" to "none"
comparison

Co-authored-by: Courtney Peverley <[email protected]>
@peverwhee
Copy link
Collaborator Author

fixed in #583

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug For issues describing bugs, or PRs fixing bugs
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@peverwhee