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

Add equals sign to list of unsafe values for plain styling #519

Merged
merged 9 commits into from Oct 17, 2019

Conversation

murtazajafferji
Copy link
Contributor

@murtazajafferji murtazajafferji commented Oct 11, 2019

According to this post and my use case, the strings with equals sign needs to be quoted:
https://stackoverflow.com/questions/19109912/do-i-need-quotes-for-strings-in-yaml
Use quotes if your value includes special characters, (e.g. :, {, }, [, ], ,, &, *, #, ?, |, -, <, >, =, !, %, @, ).

According to this post and my use case, the strings with equals sign needs to be quoted:
https://stackoverflow.com/questions/19109912/do-i-need-quotes-for-strings-in-yaml
Use quotes if your value includes special characters, (e.g. :, {, }, [, ], ,, &, *, #, ?, |, -, <, >, =, !, %, @, \).
@puzrin
Copy link
Member

puzrin commented Oct 11, 2019

Do you have a concrete sample with broken result? All such PRs should go with tests.

@puzrin
Copy link
Member

puzrin commented Oct 15, 2019

This PR will be declined if no example of which error does it fix.

@murtazajafferji
Copy link
Contributor Author

murtazajafferji commented Oct 15, 2019

If equals signs are not quoted, yaml will not be read correctly by the ruamel.yaml library when safe load is on. However, it will be loaded correctly when equals signs are quoted.

>>> import sys
>>> from  ruamel.yaml import YAML
>>> yaml = YAML(typ='safe')
>>> yaml.dump(yaml.load("="), sys.stdout)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.7/dist-packages/ruamel/yaml/main.py", line 341, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python2.7/dist-packages/ruamel/yaml/constructor.py", line 113, in get_single_data
    return self.construct_document(node)
  File "/usr/local/lib/python2.7/dist-packages/ruamel/yaml/constructor.py", line 118, in construct_document
    data = self.construct_object(node)
  File "/usr/local/lib/python2.7/dist-packages/ruamel/yaml/constructor.py", line 146, in construct_object
    data = self.construct_non_recursive_object(node)
  File "/usr/local/lib/python2.7/dist-packages/ruamel/yaml/constructor.py", line 181, in construct_non_recursive_object
    data = constructor(self, node)
  File "/usr/local/lib/python2.7/dist-packages/ruamel/yaml/constructor.py", line 743, in construct_undefined
    node.start_mark,
ruamel.yaml.constructor.ConstructorError: could not determine a constructor for the tag 'tag:yaml.org,2002:value'
  in "<byte string>", line 1, column 1
>>> yaml.dump(yaml.load("'='"), sys.stdout)
'='

@puzrin
Copy link
Member

puzrin commented Oct 15, 2019

That example is invalid. First, this must be executable code, not something, from your browser console. Second, this code works as expected:

node -e 'console.log(require("./").load("="))'
=

in js-yaml folder.

@puzrin
Copy link
Member

puzrin commented Oct 15, 2019

If problem is with ruamel.yaml library, fix should be sent there.

@murtazajafferji
Copy link
Contributor Author

murtazajafferji commented Oct 15, 2019

The example above and the one below is showing that the output of a safeDump from js-yaml will not be interpreted as valid yaml by the most popular yaml parsers for Python. This is for PyYAML:

>>> import yaml
>>> yaml.dump(yaml.load("="))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.7/dist-packages/yaml/__init__.py", line 114, in load
    return loader.get_single_data()
  File "/usr/local/lib/python2.7/dist-packages/yaml/constructor.py", line 45, in get_single_data
    return self.construct_document(node)
  File "/usr/local/lib/python2.7/dist-packages/yaml/constructor.py", line 49, in construct_document
    data = self.construct_object(node)
  File "/usr/local/lib/python2.7/dist-packages/yaml/constructor.py", line 94, in construct_object
    data = constructor(self, node)
  File "/usr/local/lib/python2.7/dist-packages/yaml/constructor.py", line 420, in construct_undefined
    node.start_mark)
yaml.constructor.ConstructorError: could not determine a constructor for the tag 'tag:yaml.org,2002:value'
  in "<string>", line 1, column 1:
    =
    ^
>>> yaml.dump(yaml.load("'='"))
"'='\n"

Copy link
Member

@puzrin puzrin left a comment

Ok, since pyyaml is reference parser, supported by spec authors, compatibility with it may make sense.

See my code comments.

test/issues/0519.js Outdated Show resolved Hide resolved
lib/js-yaml/dumper.js Outdated Show resolved Hide resolved
test/issues/0519.js Show resolved Hide resolved
@murtazajafferji
Copy link
Contributor Author

murtazajafferji commented Oct 16, 2019

@puzrin I have addressed all the comments. Do you have any other suggestions or can we complete this PR?

@puzrin
Copy link
Member

puzrin commented Oct 17, 2019

Hm, i don't know why my last note is invisible. Could you return those comments and locations back?

The only change should be added = to first one.

@murtazajafferji
Copy link
Contributor Author

murtazajafferji commented Oct 17, 2019

@puzrin Done! Btw, your comment was collapsed by default because the comment thread was marked as resolved. If you wanted it to be shown, you could press "Unresolve conversation."

@puzrin puzrin changed the title Add equals sign to list of values not considered safe for plain styling Add equals sign to list of unsafe values for plain styling Oct 17, 2019
@puzrin puzrin merged commit 2fcb465 into nodeca:master Oct 17, 2019
1 check passed
mikebryant pushed a commit to mikebryant/renovate-test-changelogjson that referenced this issue May 27, 2020
#### [\`v3.14.0\`](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md#3140---2020-05-22)

##### Changed

-   Support `safe/loadAll(input, options)` variant of call.
-   CI: drop outdated nodejs versions.
-   Dev deps bump.

##### Fixed

-   Quote `=` in plain scalars [#519](nodeca/js-yaml#519).
-   Check the node type for `!<?>` tag in case user manually specifies it.
-   Verify that there are no null-bytes in input.
-   Fix wrong quote position when writing condensed flow, [#526](nodeca/js-yaml#526).
renovate bot pushed a commit to mikebryant/renovate-test-changelogjson that referenced this issue Jun 17, 2020
##### [\`v3.14.0\`](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md#3140---2020-05-22)

##### Changed

-   Support `safe/loadAll(input, options)` variant of call.
-   CI: drop outdated nodejs versions.
-   Dev deps bump.

##### Fixed

-   Quote `=` in plain scalars [#519](nodeca/js-yaml#519).
-   Check the node type for `!<?>` tag in case user manually specifies it.
-   Verify that there are no null-bytes in input.
-   Fix wrong quote position when writing condensed flow, [#526](nodeca/js-yaml#526).
minj added a commit to minj/js-yaml that referenced this issue Jun 21, 2020
* origin/master: (37 commits)
  3.14.0 released
  Browser files rebuild
  Dev deps bump
  Travis-CI: drop old nodejs versions
  fix(loader): Add support for `safe/loadAll(input, options)`
  fix issue 526 (wrong quote position writing condensed flow)
  readme: update titelift info
  changelog format update
  Verify that there are no null-bytes in input
  Check the node type for !<?> tag in case user manually specifies it
  Add unpkg and jsdelivr fields to point to browser build
  dumper: don't quote strings with # without need
  Add equals sign to list of unsafe values for plain styling (nodeca#519)
  Use `const` where appropriate
  README: add Tidelift link
  README cleanup
  Create FUNDING.yml
  Readme: clarified “safeLoad” return type
  3.13.1 released
  Browser files rebuild
  ...
This was referenced Mar 16, 2021
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.

None yet

2 participants