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

Windows: Fix documentation strings to be raw strings #20301

Merged
merged 2 commits into from Jan 19, 2017

Conversation

dagwieers
Copy link
Contributor

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

windows modules

ANSIBLE VERSION

v2.3

SUMMARY

Especially when using Windows paths they easily get confused as escaped
sequences or unicode characters. So by default use raw strings

This fixes #20295

PS If needed we can do this for all modules so that this is consistent throughout all modules (and mistakes are reduced to a minimum).

@ansibot
Copy link
Contributor

ansibot commented Jan 16, 2017

@ansibot ansibot added affects_2.3 This issue/PR affects Ansible v2.3 community_review In order to be merged, this PR must follow the community review workflow. docs_pull_request module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. windows Windows community needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jan 16, 2017
@abadger
Copy link
Contributor

abadger commented Jan 16, 2017

The build failure here looks like my note about yaml also doing backslash escaping of strings:

2017-01-16 13:46:16 ============================================================================
2017-01-16 13:46:16 lib/ansible/modules/windows/win_robocopy.py
2017-01-16 13:46:16 ============================================================================
2017-01-16 13:46:16 TRACE:
2017-01-16 13:46:16     while scanning a double-quoted scalar
2017-01-16 13:46:16       in "<string>", line 6, column 13:
2017-01-16 13:46:16             sample: "c:\Some\Path"
2017-01-16 13:46:16                     ^
2017-01-16 13:46:16     found unknown escape character 'S'
2017-01-16 13:46:16       in "win_robocopy.RETURN", line 111, column 17:
2017-01-16 13:46:16             sample: "c:\Some\Path"

I believe the right thing to fix it is to remove the quotes from that line and let yaml treat it as an implicit string. (single quotes would also work but I believe we're moving towards implicit strings unless there's a reason [ie differentiating between floats and strings in version fields])

@dagwieers
Copy link
Contributor Author

Will fix !

@dagwieers
Copy link
Contributor Author

ready_for_review

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. community_review In order to be merged, this PR must follow the community review workflow. labels Jan 16, 2017
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Jan 17, 2017
@jhawkesworth
Copy link
Contributor

It seems there's a conflict now with win_reboot.py

If making raw string mandatory for windows examples and return strings seems like the way forward might be worth adding it to the module validation checks - https://github.com/ansible/ansible/tree/devel/test/sanity/validate-modules
I am not sure if these are run as part of the build at the moment.

Just thinking - if it doesn't cause rendering problems, perhaps all modules should use raw strings?

@petemounce
Copy link
Contributor

shipit - after rebase.

@dagwieers
Copy link
Contributor Author

Rebased. ready_for_review

@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jan 17, 2017
@jhawkesworth
Copy link
Contributor

This seems to fix things for ansible-doc, but not the rendered webdocs.
I may be out of date when it comes to how you generate the webdocs but what I did was

sudo pip install sphinx
cd docs/docsite
make clean
make

then fired up firefox to look at the rendered pages in
docs/docsite/_build/html/win_copy_module.html

Would be great to have a way of fixing these once and for all that works for both ansible-doc and the generated html.

needs_revision

@dagwieers
Copy link
Contributor Author

dagwieers commented Jan 17, 2017

I propose this gets merged so the website looks fine again.
(win_template looked fine, and it was doing exactly this)

@jhawkesworth
Copy link
Contributor

I just checked and newly rendered win_template is broken with this PR applied unfortunately :-(
Current live documentation is ok, but this looks like it was last rendered from before the @Fale made changes to remove key=value pairs (latest devel version of win_template docs no longer has ansible ad-hoc example).

So, unless I'm doing something incorrect while attempting to generate the web documentation, I still think this doesn't fix the problem for the site.

@dagwieers
Copy link
Contributor Author

@jhawkesworth Must be an amazing PR I did, because I haven't touched win_template in the PR and still it comes out wrong :-)

@jhawkesworth
Copy link
Contributor

@dagwieers Sorry trying to do too many things at once.

Can anyone else verify that the html rendering is either improved or not by this PR?

@dagwieers
Copy link
Contributor Author

dagwieers commented Jan 17, 2017

@jhawkesworth Even if the HTML rendering is not fixed by this, this PR is still going to be required to have a final fix. We don't want C:\temp to become C:<Tab>emp as input from Python, and without this fix that is exactly the result. See also the information from @abadger

Especially when using Windows paths they easily get confused as escaped
sequences or unicode characters. So by default use raw strings

This fixes ansible#20295
And some trailing whitespace fixes.
@nitzmahone nitzmahone merged commit 5b9eb92 into ansible:devel Jan 19, 2017
@ansibot ansibot added docs This issue/PR relates to or includes documentation. and removed docs_pull_request labels Mar 4, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.3 This issue/PR affects Ansible v2.3 docs This issue/PR relates to or includes documentation. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation of win_file examples render starnge
7 participants