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

Fix docs syntax highlighting errors #50836

Merged

Conversation

felixfontein
Copy link
Contributor

SUMMARY

This PR contains a couple of fixes of syntax highlighting errors reported in #50040. In most cases, the wrong lexer was used. Besides that, small problems with the Ansible output, YAML and Django lexers had to be fixed. I'll create upstream PRs for Pygments next.

Fixes #50040

CC @dagwieers @gundalow

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
COMPONENT NAME

docs

@ansibot
Copy link
Contributor

ansibot commented Jan 12, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jan 12, 2019
@bcoca bcoca added P3 Priority 3 - Approved, No Time Limitation and removed needs_triage Needs a first human triage before being processed. labels Jan 15, 2019
@felixfontein
Copy link
Contributor Author

Both Pygments PRs have been merged for the next release, i.e. once that happens, we can throw out some of the lexers.

@gundalow
Copy link
Contributor

Once Pygments has been a released do we remove the file docs/docsite/_extensions/pygments_lexer.py

@dagwieers
Copy link
Contributor

Personally, I think we need to evolve to an Ansible lexer that:

  • can distinguish between text-values and Jinja2 expressions (ie. when/until/...)
  • includes yes/no booleans

That said, I am quite happy with the progress we have made. Priority should be on easy selection of docsite version. It is the number one inconvenience for our users. It should not be that hard to do IMO.

Copy link
Contributor

@dagwieers dagwieers left a comment

Choose a reason for hiding this comment

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

LGTM

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jan 17, 2019
@felixfontein
Copy link
Contributor Author

@gundalow we can't remove it, since it contains the Ansible output lexer which isn't part of Pygments (yet). (For adding it to Pygments, I guess it needs to be more complete and better tested. Right now it's mostly good enough to run the output examples we have in the docs.)

@felixfontein
Copy link
Contributor Author

@dagwieers I didn't notice that it doesn't know about all the other boolean values. That's something easy to add, though! (I guess we should add all of them, for completeness, and try to get it into Pygments.)

About when/until/...: that's a lot more tricky (and requires a huge redesign of the lexer). It would be nice to have that eventually, though.

@dagwieers
Copy link
Contributor

@felixfontein I think the YAML lexer implements strict YAML 1.2, but we decided to allow both yes/no and true/false in documentation and examples. Adding all boolean values (i.e. on/off) is fine as well, since that's what Ansible does support anyway, but I doubt the strict YAML 1.2 is something they want to see changed.

@acozine
Copy link
Contributor

acozine commented Jan 24, 2019

@felixfontein thanks for doing this work - it looks great. As a next step, I think we need to document which lexers are available and when to use them. But meanwhile I'm going to merge this fix.

@acozine acozine merged commit f6122fb into ansible:devel Jan 24, 2019
@felixfontein felixfontein deleted the docs-syntax-highlighting-lexing-errors branch January 25, 2019 06:27
@felixfontein
Copy link
Contributor Author

@acozine The plugin adds lexers yaml, jinja (alias django), yaml+jinja, and ansible-output. The first three are already provided by Pygments itself, but we've overridden them with some improvements. Where should we document them?

@acozine
Copy link
Contributor

acozine commented Feb 1, 2019

@webknjaz
Copy link
Member

webknjaz commented Jun 5, 2019

@felixfontein does pygments_lexer need to stay vendored?

@felixfontein
Copy link
Contributor Author

@webknjaz parts of it definitely, for example AnsibleOutputLexer isn't in pygments (and there is no PR for it - I guess it should be more complete for that first). I'd have to check the rest, I don't remember which parts have already been integrated in pygments resp. whether something is missing.

@webknjaz
Copy link
Member

webknjaz commented Jun 6, 2019

@felixfontein

Sweet! Do you think it'd be possible to only keep the customized parts and just depend on the rest maintained somewhere else and installed by Pip?

@felixfontein
Copy link
Contributor Author

AnsibleYamlLexerContext/AnsibleYamlLexer are equal to YamlLexerContext/YamlLexer in the current Pygments version, and AnsibleDjangoLexer is equal to DjangoLexer, and AnsibleYamlJinjaLexer is equal to YamlJinjaLexer. So yeah, we can get rid of all of them. I can create a PR for that.

Should I add a new enough pygments version to https://github.com/ansible/ansible/blob/devel/docs/docsite/requirements.txt ? Or where should I add that requirement?

@felixfontein
Copy link
Contributor Author

(FYI: the required patches so that the code is actually equivalent are in Pygments 2.4.0 and newer.)

@webknjaz
Copy link
Member

webknjaz commented Jun 6, 2019

@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. P3 Priority 3 - Approved, No Time Limitation shipit This PR is ready to be merged by Core 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.

Docs code-blocks of task/playbook output should not get err class
7 participants