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

Docs: add notes on command line quoting #1844

Merged
merged 5 commits into from Nov 5, 2021

Conversation

Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented Oct 4, 2021

This PR adds two notes to the documentation on command-line overrides.
Related to #1843.

@Jasha10 Jasha10 requested a review from jieru-hu October 4, 2021 21:32
@omry
Copy link
Collaborator

omry commented Oct 4, 2021

I suspect | can be supported in unquoted strings as we are not using it for anything else in the grammar.

@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 5, 2021
Comment on lines 154 to 159
$ python my_app.py +foo="{a: 10}" # the string `+foo={a: 10}` is passed to Hydra by bash
foo:
a: 10
Copy link
Contributor

@jieru-hu jieru-hu Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be confusing. since we recommend quote key=value pare in single quote, maybe update this to

python my_app.py '+foo={a: 10}'

if we update the example like above, we can probably skip the comments

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in d300016.

Copy link
Contributor

@jieru-hu jieru-hu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Jasha10 for updating the doc!

This is one of the most common questions we get - here is a list of tips from Omry (copy pasted from discussions before), could you also include them in the doc?

- quote the whole key=value pair with a single quote (this quote is for the benefit of the shell).
- do not quote keys.
- only quote values if they contain a space (it will work if you always quote values, but it will turn numbers into strings)
- when you are quoting values, use double quotes to avoid collision with the outer single quoted consumed by the shell.

@Jasha10 Jasha10 marked this pull request as draft October 10, 2021 20:10
@Jasha10 Jasha10 changed the title Docs: add notes on command line quoting and bar symbol Docs: add notes on command line quoting Nov 4, 2021
@Jasha10
Copy link
Collaborator Author

Jasha10 commented Nov 4, 2021

Rebased to incorporate recent CI fixes on the main branch.

@Jasha10
Copy link
Collaborator Author

Jasha10 commented Nov 4, 2021

This is one of the most common questions we get - here is a list of tips from Omry (copy pasted from discussions before), could you also include them in the doc?
...

Updated in df625f8.

@Jasha10 Jasha10 marked this pull request as ready for review November 4, 2021 12:23
Copy link
Contributor

@pixelb pixelb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you've squashed the 1.1 update into this also.
In general it's probably best to cherry-pick a separate commit to the 1.1 branch

example above).
- When you are quoting values, use double quotes to avoid collision with the
outer single quoted consumed by the shell.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are good basic rules that should cater for almost all cases.

BTW double quoting in the shell is more general than single quoting, as you can specify any character through the use of escapes (you can't have literal single quotes within single quoted args for example). But I wouldn't bother mentioning that in the doc as it's shell specific

@jieru-hu
Copy link
Contributor

jieru-hu commented Nov 4, 2021

@pixelb
right now we only deploy the doc changes on main branch (link), so I think it is OK to update the doc changes in one commit, and probably fine not to cherry-pick to 1.1_branch (since those changes won't be deployed in the current setup.) What do you think?

Copy link
Contributor

@jieru-hu jieru-hu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lg! thanks @Jasha10 !

@Jasha10 Jasha10 merged commit 8feeb05 into facebookresearch:main Nov 5, 2021
@Jasha10 Jasha10 deleted the closes1843 branch November 5, 2021 11:58
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.

None yet

5 participants