Skip to content

grammar: Allow pipe character | in unquoted strings#1850

Merged
Jasha10 merged 4 commits intofacebookresearch:mainfrom
Jasha10:allow_pipe_in_unquoted_strings
Oct 22, 2021
Merged

grammar: Allow pipe character | in unquoted strings#1850
Jasha10 merged 4 commits intofacebookresearch:mainfrom
Jasha10:allow_pipe_in_unquoted_strings

Conversation

@Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented Oct 10, 2021

This PR is a follow-up to Omry's comment here suggesting that Hydra's override grammar allow the pipe character | in unquoted strings.

If we decide to move forward with this PR, I think we should update OmegaConf's grammar too.

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 to me, but yes we'll need the same change in OmegaConf's grammar as well.

@odelalleau
Copy link
Collaborator

Oh, can you add a news fragment as well?

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 13, 2021
@odelalleau odelalleau self-requested a review October 14, 2021 17:27
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.

Thanks for the changes, sorry I had forgotten something else in my first review: the parser grammar is included in the doc (https://hydra.cc/docs/advanced/override_grammar/basic) and the doc thus needs to be also updated.

@Jasha10
Copy link
Collaborator Author

Jasha10 commented Oct 19, 2021

Aah yes, good point. Updated in a32f168.

@odelalleau odelalleau self-requested a review October 19, 2021 12:44
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.

Go! (preferably after CI is fixed)

@Jasha10 Jasha10 force-pushed the allow_pipe_in_unquoted_strings branch from a32f168 to ee14dc5 Compare October 21, 2021 17:12
@Jasha10 Jasha10 merged commit 949ad62 into facebookresearch:main Oct 22, 2021
@Jasha10 Jasha10 deleted the allow_pipe_in_unquoted_strings branch October 22, 2021 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants