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.

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

Merged
merged 8 commits into from Oct 19, 2021

Conversation

Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented Oct 12, 2021

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.

@Jasha10 Jasha10 mentioned this pull request Oct 12, 2021
Copy link
Collaborator

@odelalleau odelalleau left a 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 |?):

(maybe there should be an UNQUOTED_SPECIAL string like in Hydra that all these tests would re-use?)

omegaconf/grammar/OmegaConfGrammarParser.g4 Outdated Show resolved Hide resolved
@Jasha10
Copy link
Collaborator Author

Jasha10 commented Oct 14, 2021

(maybe there should be an UNQUOTED_SPECIAL string like in Hydra that all these tests would re-use?)

Done in 5f0fad0.

Copy link
Collaborator

@odelalleau odelalleau left a 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

tests/test_grammar.py Show resolved Hide resolved
tests/test_grammar.py Show resolved Hide resolved
@odelalleau odelalleau self-requested a review October 17, 2021 16:12
Copy link
Collaborator

@odelalleau odelalleau left a 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...

@Jasha10
Copy link
Collaborator Author

Jasha10 commented Oct 18, 2021

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 nox and pip.

@Jasha10 Jasha10 force-pushed the allow_pipe_in_unquoted_strings_omegaconf branch from 0ac2d37 to 42958b1 Compare October 19, 2021 20:15
@Jasha10 Jasha10 merged commit 055eef2 into omry:master Oct 19, 2021
@Jasha10 Jasha10 deleted the allow_pipe_in_unquoted_strings_omegaconf branch October 19, 2021 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants