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

docsite: remove lexers which have been fixed in Pygments 2.4.0 #57508

Merged
merged 9 commits into from Jun 18, 2019

Conversation

@felixfontein
Copy link
Contributor

commented Jun 6, 2019

SUMMARY

Removes the lexers (YAML, Django, and YAML+Jinja) which were included in a fixed form from Pygments. The fixes have been included in Pygments 2.4.0, so it is necessary to require that (or a newer) Pygments version.

There is still some code from Pygments left, namely the states from the JSON lexer, which are needed by AnsibleOutputPrimaryLexer.

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

docs/docsite/_extensions/pygments_lexer.py

@felixfontein felixfontein requested a review from webknjaz Jun 6, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

@ansibot

This comment was marked as resolved.

Copy link
Contributor

commented Jun 6, 2019

The test ansible-test sanity --test docs-build [explain] failed with 6 errors:

docs/docsite/rst/index.rst:36:0: literal-block-lex-error: Could not lex literal_block as "YAML+Jinja". Highlighting skipped.
docs/docsite/rst/index.rst:84:0: literal-block-lex-error: Could not lex literal_block as "yaml+jinja". Highlighting skipped.
docs/docsite/rst/index.rst:110:0: literal-block-lex-error: Could not lex literal_block as "YAML+Jinja". Highlighting skipped.
docs/docsite/rst/index.rst:176:0: literal-block-lex-error: Could not lex literal_block as "yaml+jinja". Highlighting skipped.
docs/docsite/rst/index.rst:234:0: literal-block-lex-error: Could not lex literal_block as "yaml+jinja". Highlighting skipped.
docs/docsite/rst/index.rst:407:0: literal-block-lex-error: Could not lex literal_block as "yaml+jinja". Highlighting skipped.

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Jun 6, 2019

@webknjaz webknjaz requested review from acozine and mattclay Jun 6, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Hmm, in the tests only an older version of Pygments is installed (00:27 Requirement already satisfied: Pygments>=2.0 in /usr/local/lib/python3.6/dist-packages (from sphinx<1.8->-c test/runner/requirements/constraints.txt (line 7)) (2.3.1)). I guess that's why it fails to lex some of the YAML+Jinja blocks.

Who exactly is using the docsite requirements.txt file?

@webknjaz

This comment has been minimized.

@ansibot ansibot removed the needs_triage label Jun 6, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

@ansibot ansibot added the test label Jun 7, 2019

@ansibot

This comment was marked as resolved.

Copy link
Contributor

commented Jun 7, 2019

The test ansible-test sanity --test docs-build [explain] failed with 5 errors:

docs/docsite/rst/index.rst:36:0: literal-block-lex-error: Could not lex literal_block as "YAML+Jinja". Highlighting skipped.
docs/docsite/rst/index.rst:110:0: literal-block-lex-error: Could not lex literal_block as "YAML+Jinja". Highlighting skipped.
docs/docsite/rst/index.rst:176:0: literal-block-lex-error: Could not lex literal_block as "yaml+jinja". Highlighting skipped.
docs/docsite/rst/index.rst:234:0: literal-block-lex-error: Could not lex literal_block as "yaml+jinja". Highlighting skipped.
docs/docsite/rst/index.rst:407:0: literal-block-lex-error: Could not lex literal_block as "yaml+jinja". Highlighting skipped.

click here for bot help

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

One less failure, but still five left... I'll take a closer look over the weekend.

@acozine

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

@felixfontein this is odd - the test is reporting YAML/jinja block errors on the main TOC (docs/docsite/rst/index.rst), on line numbers that don't exist in that file. It looks like it's objecting to the toctree directives.

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@acozine it looks like the line number reporting is partially broken - I've seen this before, and it's really annoying to find out where these problems actually come from. They are probably from some YAML/Jinja listing somewhere...

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Fortunately, a local build produced exact file names and line numbers, so analyzing this was pretty easy. It turns out that all five places have broken YAML+Jinja2. I guess the "old" lexer also didn't like them; no idea why everything worked before though (maybe the sanity checks only complain about errors when they are somehow related to the PR?).

This is an example which will fail:

.. code-block:: text

# FAILS
tempdir: "C:\Windows\Temp"

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jun 11, 2019

Author Contributor

This part broke the example: it is not valid YAML. Because this error is intentional, I broke up the example into three parts: two correct YAML+Jinja2, and this one formatted as text inbetween.

invocation output for the above ``Registry`` task::
invocation output for the above ``Registry`` task:

.. code-block:: ansible-output

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jun 11, 2019

Author Contributor

The code below was not YAML+Jinja2, but something that should be lexed as ansible-output.

@@ -33,7 +33,7 @@ Comparing ``loop`` and ``with_*``
you would need::

loop: [1, [2,3] ,4] | flatten(1)
loop: "{{ [1, [2,3] ,4] | flatten(1) }}"

This comment has been minimized.

Copy link
@felixfontein

felixfontein Jun 11, 2019

Author Contributor

Running the old code with Ansible resulted in an error. It has to be written in this form to work.

For the example itself, it might be better to rewrite this as

loop: "{{ data | flatten(1) }}"
vars:
  data: [1, [2, 3] ,4]

At least that's what I would use in a playbook, because it's much better readable and extensible.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@acozine
Copy link
Contributor

left a comment

Thanks @felixfontein for updating the pygments version and fixing the code snippets.

@felixfontein felixfontein force-pushed the felixfontein:pygments-devendorize branch from 3c6c019 to 592d55c Jun 18, 2019

@acozine acozine merged commit 505c992 into ansible:devel Jun 18, 2019

1 check passed

Shippable Run 128219 status is SUCCESS.
Details

@felixfontein felixfontein deleted the felixfontein:pygments-devendorize branch Jun 18, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

@webknjaz @acozine should this be backported?

@webknjaz

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

I think it could be useful

felixfontein added a commit to felixfontein/ansible that referenced this pull request Jun 19, 2019

docsite: remove lexers which have been fixed in Pygments 2.4.0 (ansib…
…le#57508)

* Remove lexers which have been fixed in Pygments 2.4.0.
* Add Pygments >= 2.4.0 to test runner.
* Fix pages that triggered lexer errors.

Co-Authored-By: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
(cherry picked from commit 505c992)

felixfontein added a commit to felixfontein/ansible that referenced this pull request Jun 19, 2019

docsite: remove lexers which have been fixed in Pygments 2.4.0 (ansib…
…le#57508)

* Remove lexers which have been fixed in Pygments 2.4.0.
* Add Pygments >= 2.4.0 to test runner.
* Fix pages that triggered lexer errors.

Co-Authored-By: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
(cherry picked from commit 505c992)
@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

I created backports for 2.7 (#58101) and 2.8 (#58100). The 2.7 bugfix leaves essentially an empty file; any opinions on whether I should remove it completely, or leave the empty hull?

@webknjaz

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

removal is fine with me unless RM and @acozine have any other thoughts.

abadger added a commit that referenced this pull request Jun 20, 2019

docsite: remove lexers which have been fixed in Pygments 2.4.0 (#57508)
* Remove lexers which have been fixed in Pygments 2.4.0.
* Add Pygments >= 2.4.0 to test runner.
* Fix pages that triggered lexer errors.

Co-Authored-By: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
(cherry picked from commit 505c992)

felixfontein added a commit to felixfontein/ansible that referenced this pull request Jun 25, 2019

docsite: remove lexers which have been fixed in Pygments 2.4.0 (ansib…
…le#57508)

* Remove lexers which have been fixed in Pygments 2.4.0.
* Add Pygments >= 2.4.0 to test runner.
* Fix pages that triggered lexer errors.

Co-Authored-By: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
(cherry picked from commit 505c992)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.