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

Allow colon in a plain scalar in a flow context #45

Merged
merged 2 commits into from Feb 8, 2017

Conversation

Projects
None yet
7 participants
@dbeer1
Copy link
Contributor

dbeer1 commented Dec 7, 2016

This pull is analogous to yaml/libyaml#28. It changes the behavior of pyyaml the same way the other pull changes the behavior of libyaml.

@sigmavirus24 sigmavirus24 self-assigned this Dec 7, 2016

@sigmavirus24

This comment has been minimized.

Copy link
Contributor

sigmavirus24 commented Dec 7, 2016

Hey @dbeer1!

Refresh my memory, is this valid for scalars even in 1.1 or just 1.2? At the moment, pyyaml and libyaml only support 1.1.

@@ -1,7 +1,7 @@
%YAML 1.1
---
!!map {
? !!str "foo"
? !!str "foo :"

This comment has been minimized.

Copy link
@sigmavirus24

sigmavirus24 Dec 7, 2016

Contributor

Did this fail prior to this change?

This comment has been minimized.

Copy link
@dbeer1

dbeer1 Dec 8, 2016

Author Contributor

It passed, but the key was parsed as "foo". Note that pyyaml and libyaml behave differently in this edge case, libyaml rejects any colon in a flow context without a space afterwards, while pyyaml allows ',[]{}' after a colon as well.

Compare https://github.com/yaml/pyyaml/blob/master/lib/yaml/scanner.py#L1301 vs https://github.com/yaml/libyaml/blob/master/src/scanner.c#L3438

This comment has been minimized.

Copy link
@dbeer1

dbeer1 Dec 8, 2016

Author Contributor

I reverted this test and behavior with regard to empty values in flow mappings in the latest commit, fyi.

@adrienverge

This comment has been minimized.

Copy link

adrienverge commented Dec 9, 2016

I just check in the specs, and it appears to be the same for 1.1 and 1.2. From 1.1:

Plain scalars can never contain any tab characters. They also must not contain the “: ” and “ #” character sequences as these combinations cause ambiguity with key: value pairs and comments. Inside flow collections, plain scalars are further restricted to avoid containing the “[”, “]”, “{”, “}” and “,” characters as these would cause ambiguity with the flow collection structure (hence the need for the flow-in context and the flow-out context).

So ": " (notice the space) is forbidden, but ":" isn't.

Related issue: adrienverge/yamllint#30

@adrienverge

This comment has been minimized.

Copy link

adrienverge commented Feb 6, 2017

Hi @sigmavirus24, do you have any update on this?

@ingydotnet

This comment has been minimized.

Copy link
Member

ingydotnet commented Feb 8, 2017

Kirill strayed from the spec a few times when he thought it was really not the right thing to do. I suspect this might be the case. {foo:bar} is pretty ambiguous. Even {url:http://foo.io} is weird looking at best. However [ http://127.0.0.1:1234, http://127.0.0.1:2345 ] is not.

I just tested this using the new YAML Editor: http://pasteboard.co/vYxs4oX8D.png

Seems like most frameworks accept it, except the libyaml ones. Since it is in the spec and EveryonesDoingIt™, I move to add this to libyaml and all its bindings.

@sigmavirus24 sigmavirus24 merged commit c5b135f into yaml:master Feb 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sigmavirus24

This comment has been minimized.

Copy link
Contributor

sigmavirus24 commented Feb 8, 2017

Thanks for confirming that detail @ingydotnet

@dbeer1

This comment has been minimized.

Copy link
Contributor Author

dbeer1 commented Feb 8, 2017

Thanks! Note that the C binding pull request (yaml/libyaml#28) needs to be updated to follow the modified logic here with regard to a missing value after a colon, eg {a:} -> { "a" : null } instead of {a:} -> { "a:" : null }

@bobvandevijver

This comment has been minimized.

Copy link

bobvandevijver commented Feb 27, 2017

@sigmavirus24 Can a new release be drafted with this change? I'm waiting for it so my I can update my docker containers without moving from the simple pip install command 😃

@dbeer1 dbeer1 deleted the dbeer1:feature/colon_in_flow_context branch Jul 19, 2017

@tobias-93

This comment has been minimized.

Copy link

tobias-93 commented Jul 26, 2017

@sigmavirus24 Would it be possible to make a new release with these changes? Thank you!

@asomov

This comment has been minimized.

Copy link
Contributor

asomov commented Dec 23, 2017

Hi all, I see that the change was already merged.
I think that it is still incomplete. It should be finished before releasing.

  1. No new tests have been added. There should be at least a test for {a:1} and {a:} (because they are counter intuitive)
  2. The file spec-08-13.skip-ext should be removed to let the test fail. Then either the implementation or spec-08-13.canonical should be adapted.

We have a similar request in SnakeYAML
I would prefer to solve the questions before SnakeYAML follows the change.

asomov added a commit to asomov/pyyaml that referenced this pull request Mar 2, 2018

@asomov

This comment has been minimized.

Copy link
Contributor

asomov commented Mar 3, 2018

The same change is implemented in SnakeYAML: https://bitbucket.org/asomov/snakeyaml/issues/397

@ingydotnet

This comment has been minimized.

Copy link
Member

ingydotnet commented Jun 24, 2018

@asomov The PyYAML release of this is being planned to happen in the next 48 hours. Would you mind dropping by #pyyaml on irc.freenode.net to discuss?

@ingydotnet ingydotnet removed this from Added to release/5/1 branch in 5.1 Release Planning Feb 23, 2019

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.