-
Notifications
You must be signed in to change notification settings - Fork 2
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
Unit test for parameter interpolation failure #105
Conversation
Codecov Report
@@ Coverage Diff @@
## master #105 +/- ##
==========================================
+ Coverage 78.58% 78.75% +0.16%
==========================================
Files 24 24
Lines 2358 2358
==========================================
+ Hits 1853 1857 +4
+ Misses 505 501 -4
Continue to review full report at Codecov.
|
vistautils/parameters.py
Outdated
@@ -1216,7 +1216,7 @@ def _check_all_keys_strings(mapping: Mapping, path=None): | |||
if isinstance(val, Mapping): | |||
YAMLParametersLoader._check_all_keys_strings(val) | |||
|
|||
_INTERPOLATION_REGEX = re.compile(r"%([\w\.]+)%") | |||
_INTERPOLATION_REGEX = re.compile(r"%([\w.\-_]+)%") |
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.
If I check out this PR but then revert the regex change, this still passes. Underscore should be covered by \w
, so that leaves \w\.
versus \w.\-
. Here are my tests:
In [17]: test_str = "foo-bar_baz.meep"
In [18]: re.findall(r"[\w\.]+", test_str)
Out[18]: ['foo', 'bar_baz.meep']
In [19]: re.findall(r"[\w.\-]+", test_str)
Out[19]: ['foo-bar_baz.meep']
In [20]: re.findall(r"[\w\-]+", test_str)
Out[20]: ['foo-bar_baz', 'meep']
We should have a test for this if we don't already.
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.
Addressed in ea54110
I've confirmed that this effectively tests the current regex and breaks when reverting to the old one. |
No description provided.