Skip to content

enclose YAML strings containing % with quotes #4030

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

Merged
merged 2 commits into from
Jul 29, 2014

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Jul 16, 2014

Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets #1886, #4029

Thanks @iamdto, did you spot some other occurences in the docs?

@@ -499,7 +499,7 @@ Full default Configuration
collect: true
only_exceptions: false
only_master_requests: false
dsn: file:%kernel.cache_dir%/profiler
dsn: "file:%kernel.cache_dir%/profiler"
Copy link
Member

Choose a reason for hiding this comment

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

to be exact, YAML 1.1 has an issue only when % starts the values, so this change is not necessary

@iamdto
Copy link
Contributor

iamdto commented Jul 16, 2014

A quick search on the documentation give me this:

The last one in the JSON example is caused by the three dots in the file I think.

It looks like everytime the syntax highlighter fails, it sets the code dark on bright. Was this the case before the new version of the website? @javiereguiluz any inputs on this?

@stof
Copy link
Member

stof commented Jul 16, 2014

@iamdto when the syntax highlighter fails, the code was not highlighted either previously (this is not related to the website itself, but to the Python tool used to render the doc). However, the background was still black in the old website IIRC.

@iamdto
Copy link
Contributor

iamdto commented Jul 16, 2014

Yes, exactly, that's what I meant. Even when syntax highlighting failed the code was still bright on black.

@xabbuh
Copy link
Member Author

xabbuh commented Jul 16, 2014

Indeed, the old style rendered it like a text code block.

@xabbuh
Copy link
Member Author

xabbuh commented Jul 16, 2014

Changed the examples mentioned by @iamdto and updated the changeset according to @stof's comment.

@@ -119,15 +119,15 @@ or this:

The output will look like this:

.. code-block:: text
.. code-block:: yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

In this file the issue is not about YAML but more about the first JSON example (see the rendered version on the site). In fact in this part we're talking about the output of the command above, so I think we should let it be a text code block.

Copy link
Member

Choose a reason for hiding this comment

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

Given that this is the output of the command, I agree we should keep it highlited as text, not as yaml (and we should not add the quotes which are not part of the output).

Highlighting the YAML could be confusing if some user then expect the command output to be highlighted

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true. I didn't look carefully enough. This change is now reverted.

For the JSON example: It's not rendered properly because of the ... the JSON is no longer valid. Though I'm not sure how to work with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, we don't have that much solutions:

  1. We fix the syntax of all JSON examples in the documentation (though it could happen on other languages too).
  2. We had back the light on dark theme for the default syntax highlighting.
  3. We keep it like this.

@xabbuh
Copy link
Member Author

xabbuh commented Jul 17, 2014

@javiereguiluz What do you think about the default code block style?

@wouterj
Copy link
Member

wouterj commented Jul 17, 2014

The xml code containing the three dots in a tag (e.g. <container ...>) are broken too. See also #1886 (comment)

Pygment uses the YAML 1.1 standard to parse YAML code being highlighted.
A string value cannot start with the percent sign or the @ character
in YAML 1.1 without being quoted. To highlight the YAML code blocks
properly, strings starting with the percent sign are now enclosed in
double quotes.
@xabbuh
Copy link
Member Author

xabbuh commented Jul 18, 2014

@wouterj Thanks for the hint. Most of the code blocks you found have been fixed in the meantime in the 2.3 branch. The rest should be fixed with this PR.

@javiereguiluz
Copy link
Member

@xabbuh I've added this bug to the symfony.com backlog. It will be probably fixed soon because it can be easily fixed. Thank you all for reporting this error!

@xabbuh
Copy link
Member Author

xabbuh commented Jul 18, 2014

@javiereguiluz Thank you!

The ellipses were used as placeholders in XML element tags. Thus, the
XML wasn't well-formed and therefore would have never been rendered
properly.
@javiereguiluz
Copy link
Member

This error has been fixed and it will be online after the next deploy.

@xabbuh
Copy link
Member Author

xabbuh commented Jul 21, 2014

👍

@weaverryan
Copy link
Member

Very good changes, looks great to me :). Thanks Christian!

@weaverryan weaverryan merged commit c1c4bc8 into symfony:2.3 Jul 29, 2014
weaverryan added a commit that referenced this pull request Jul 29, 2014
This PR was merged into the 2.3 branch.

Discussion
----------

enclose YAML strings containing % with quotes

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | all
| Fixed tickets | #1886, #4029

Thanks @iamdto, did you spot some other occurences in the docs?

Commits
-------

c1c4bc8 remove "..." from XML element tags
dbc02a2 quote YAML strings starting with % or @ characters
@xabbuh xabbuh deleted the issue-4029 branch July 29, 2014 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants