-
Notifications
You must be signed in to change notification settings - Fork 122
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
Interpolation grammar: allow pipe |
in unquoted strings
#799
Interpolation grammar: allow pipe |
in unquoted strings
#799
Conversation
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.
Can you also edit (adding both ?
and |
?):
omegaconf/omegaconf/grammar_parser.py
Line 30 in 32e82d3
_arg = "[a-zA-Z_0-9/\\-\\+.$%*@]+" # string representing a resolver argument omegaconf/tests/test_grammar.py
Line 184 in 32e82d3
fr"{{a0-null-1-3.14-NaN- {TAB}-true-False-/\+.$%*@\(\)\[\]\{{\}}\:\=\ \{TAB}\,:0}}", omegaconf/tests/test_grammar.py
Line 732 in 32e82d3
supported_chars = r"abc123_/:-\+.$%*@"
(maybe there should be an UNQUOTED_SPECIAL
string like in Hydra that all these tests would re-use?)
Done in 5f0fad0. |
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.
Just some minor suggestions
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, thanks! Just curious about the CI errors, that seem unrelated => I tried to restart the build but somehow it didn't like it...
Yes, I believe the CI errors are unrelated. Jieru and I were trying to fix them in PR #800. I'm not yet sure exactly what's going on -- it seems related to interaction between |
0ac2d37
to
42958b1
Compare
This PR adds support for the pipe character
|
to appear unquoted in OmegaConf's interpolation grammar.Related to facebookresearch/hydra#1850.
For good measure, I've also added a test for the pipe appearing in quoted strings.