-
Notifications
You must be signed in to change notification settings - Fork 25
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.
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
Fix nonumerics in for-loops #41
Conversation
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
==========================================
+ Coverage 97.16% 97.17% +0.01%
==========================================
Files 12 12
Lines 1902 1914 +12
==========================================
+ Hits 1848 1860 +12
Misses 54 54
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my end @thisac! Only suggestion is to add another test, using the boolean for loop variable within the for loop.
def test_for_bool(self, parse_input_mocked_metadata): | ||
"""Test that a for-loop over a list containing bools is parsed correctly""" | ||
bb = parse_input_mocked_metadata( | ||
"for bool b in [True, False]\n\tMeasureFock() | 0" | ||
) | ||
assert np.all( | ||
bb._forvar["b"] == np.array([True, False]) | ||
) | ||
|
||
def test_for_str(self, parse_input_mocked_metadata): | ||
"""Test that a for-loop over a list containing strings is parsed correctly""" | ||
bb = parse_input_mocked_metadata( | ||
'for str s in ["one", "two"]\n\tMeasureFock() | 0' | ||
) | ||
assert np.all( | ||
bb._forvar["s"] == np.array(["one", "two"]) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests, although, should they also be testing for the case where the boolean for loop variable is used within the loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the test to use the bool inside the loop. Not sure if there's another way of testing it. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Theo!
Using bool or string in for loop iterators isn't working properly. Currently, it returns None, instead of the wanted string or bool. This PR fixes the issue, so that e.g. the following works.