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

INI: values with spaces can be single or double quoted #77369

Merged
merged 2 commits into from
Apr 1, 2022

Conversation

akorn
Copy link

@akorn akorn commented Mar 28, 2022

Ideally the page should enumerate all characters whose presence requires the value to be quoted (I'm guessing that includes quotes and possibly backslashes?).

Also: maybe you can also escape whitespace with a backslash, not just quote it?

SUMMARY

The page said "When declared inline with the host, INI values are interpreted as Python literal structures (strings, numbers, tuples, lists, dicts, booleans, None). Host lines accept multiple key=value parameters per line. Therefore they need a way to indicate that a space is part of a value rather than a separator." but failed to say what that way was. I added quoting as one possible way.

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

inventory(?)

ADDITIONAL INFORMATION

None

Ideally the page should enumerate all characters whose presence requires the value to be quoted (I'm guessing that includes quotes and possibly backslashes?).

Also: maybe you can also escape whitespace with a backslash, not just quote it?
@ansibot
Copy link
Contributor

ansibot commented Mar 28, 2022

Thanks for your Ansible docs contribution! We talk about Ansible documentation on maxtrix at #docs:ansible.im and on libera IRC at #ansible-docs if you ever want to join us and chat about the docs! We meet there on Tuesdays (see the Ansible calendar) and welcome additions to our weekly agenda items - scroll down to find the upcoming agenda and add a comment to put something new on that agenda.

click here for bot help

@ansibot ansibot added affects_2.13 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. docs_only All changes are to files within the docs/docsite/ directory needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Mar 28, 2022
Copy link
Contributor

@briantist briantist 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!
@sivel provided the answer in IRC originally as well.

I am not sure about the questions raised by @akorn though, re: escaping on other characters.

@samccann
Copy link
Contributor

Ok we can leave it open for a few days to see if @sivel or someone can answer that final question. Thanks @akorn for the ansible docs fix and welcome to Ansible!

@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Mar 29, 2022
@sivel
Copy link
Member

sivel commented Mar 29, 2022

I think it's just sufficient to say that the value can be quoted. I don't think we should call out both single and double quotes. If we want, we can link to documentation for shlex here.

https://docs.python.org/3/library/shlex.html#parsing-rules

We operate in POSIX mode.

However, I don't think we should try to duplicate the info from the python docs into our docs. Which is why I think just saying "The value can be quoted" is sufficient.

@akorn
Copy link
Author

akorn commented Mar 29, 2022

I think it's just sufficient to say that the value can be quoted. I don't think we should call out both single and double quotes. If we want, we can link to documentation for shlex here.

https://docs.python.org/3/library/shlex.html#parsing-rules

That's a good idea, a link should definitely be included.

However, I don't think we should try to duplicate the info from the python docs into our docs. Which is why I think just saying "The value can be quoted" is sufficient.

I agree it's not necessary to duplicate the Python documentation, but I think it's useful to explicitly state what kinds of quotes will work. As a user, if I were reading that document and it just said "can be quoted" I would be annoyed because it didn't say how and I would need to find out by experimenting. "Single or double" isn't excessively wordy, isn't likely to change, and doesn't duplicate too much of the Python documentation.

@samccann
Copy link
Contributor

Hi @akorn - can you add the link and POSIX note to this PR? As for whether to have just 'quoted' vs 'single or double quoted'... I would lean toward @sivel's comment that saying quoted is enough. There is only single or double quoted options, right? But if you feel strongly that it helps the user to state it explicitly, I'm fine with that as well.

Thanks!

I also reduced the wordiness ("single quoted or double quoted" -> "quoted (single or double)"
@akorn
Copy link
Author

akorn commented Apr 1, 2022

Hi @akorn - can you add the link and POSIX note to this PR? As for whether to have just 'quoted' vs 'single or double quoted'... I would lean toward @sivel's comment that saying quoted is enough. There is only single or double quoted options, right? But if you

Yes, there are only these two options, but either both work or only one of them works. I think adding three words to point out that both work is better than leaving the user guessing, or having to find out by trial and error, or having to check the shlex documentation.

I made it "quoted (single or double)", and added the shlex parsing rules link (correctly, I hope -- I'm not terribly good with rst).

@samccann samccann merged commit ac56647 into ansible:devel Apr 1, 2022
@samccann
Copy link
Contributor

samccann commented Apr 1, 2022

Thanks @akorn for the Ansible docs fix!

@akorn akorn deleted the patch-1 branch April 1, 2022 16:13
samccann pushed a commit to samccann/ansible that referenced this pull request Apr 7, 2022
@ansible ansible locked and limited conversation to collaborators Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.13 core_review In order to be merged, this PR must follow the core review workflow. docs_only All changes are to files within the docs/docsite/ directory docs This issue/PR relates to or includes documentation. new_contributor This PR is the first contribution by a new community member. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants