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

Revert removing of open_ended after top level plain scalar #122

Merged
merged 2 commits into from
Jun 30, 2018

Conversation

perlpunk
Copy link
Member

@perlpunk perlpunk commented Jun 29, 2018

See also issue #60

This reverts commit 56400d9.

Currently tests will fail because we have the yaml-test-suite with very strict
tests that check for explicit document start/end markers.

So the following tests need to be skipped for now: 27NA 35KP 4V8U 9KAX P76L

The problematic case was that it created the following YAML:

--- foo
%YAML 1.1
--- bar

But you would need ... after `foo' here. Only when quoted you can leave out the end marker:

--- "foo"
%YAML 1.1
--- bar

In YAML 1.2 you always have to add the end marker, so I think it's a good thing.

But I know that the end marker also gets added to cases where it is not needed. Maybe I can have a look into that.

See also issue #60

Revert "This code is not needed and breaks tests"

This reverts commit 56400d9.
@perlpunk
Copy link
Member Author

@ingydotnet Could you add 27NA 35KP 4V8U 9KAX P76L to the emitter test skip list? (Or remove them from the test whitelist)
And when doing that, maybe you can also change the test-suite to fetch the yaml-test-suite data release instead of a commit id?

@LocutusOfBorg
Copy link

can you please also apply the second part of the patch on #60?

@perlpunk
Copy link
Member Author

@LocutusOfBorg yeah, I was looking at it. Funnily I read this issue only after I searched for the cause of the test failures myself =)

I have to first run a whole lotta tests on it, with pyyaml, with perl's YAML::LibYAML, and yaml-test-suite, to make sure it isn't breaking something else.
Not sure how far I can get today...

@perlpunk
Copy link
Member Author

perlpunk commented Jun 29, 2018

I added the patch, thanks @jrtc27

@ingydotnet the tests are passing again, no need to skip them.
Also YAML::LibYAML tests are all passing again.
pyyaml tests are also passing, although they were already passing for me without the patch from @jrtc27

As a side note:
I can actually think of cases where I would want to add the trailing ..., but these have different conditions.
For example if you have this stream:

foo: |+
  a
  b

...

Then the document-end marker should be emitted, even if not requested, because the exact number of trailing lines needs to be preserved.
I should create a new issue for that.

@jrtc27
Copy link

jrtc27 commented Jun 29, 2018

You missed the 7 on my username for the second commit :)

@perlpunk
Copy link
Member Author

@jrtc27 oops, will fix it ;-)
I'll let appveyor finish its tests first though

(only when explicitly requested)

@jrtc27++ for the patch.

See #60
@perlpunk perlpunk force-pushed the revert-open-ended branch from 8e33199 to 56f4b17 Compare June 29, 2018 22:16
@perlpunk
Copy link
Member Author

fixed typo and force pushed

@ingydotnet
Copy link
Member

Applied. Pushed. Thanks!

@perlpunk perlpunk deleted the revert-open-ended branch March 11, 2019 23:13
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Apr 1, 2019
Changes
=======

* yaml/libyaml#95 -- build: do not install config.h
* yaml/libyaml#97 -- appveyor.yml: fix Release build
* yaml/libyaml#103 -- Remove unused code in yaml_document_delete
* yaml/libyaml#104 -- Allow colons in plain scalars inside flow collections
* yaml/libyaml#109 -- Fix comparison in tests/run-emitter.c
* yaml/libyaml#117 -- Fix typo error
* yaml/libyaml#119 -- The closing single quote needs to be indented...
* yaml/libyaml#121 -- fix token name typos in comments
* yaml/libyaml#122 -- Revert removing of open_ended after top level plain scalar
* yaml/libyaml#125 -- Cherry-picks from PR 27
* yaml/libyaml#135 -- Windows/C89 compatibility
* yaml/libyaml#136 -- allow override of Windows static lib name
Jehops pushed a commit to Jehops/freebsd-ports-legacy that referenced this pull request Apr 11, 2019
Fix portlint errors in Makefile

Changes in 0.2.2:
* yaml/libyaml#95 -- build: do not install config.h
* yaml/libyaml#97 -- appveyor.yml: fix Release build
* yaml/libyaml#103 -- Remove unused code in yaml_document_delete
* yaml/libyaml#104 -- Allow colons in plain scalars inside flow collections
* yaml/libyaml#109 -- Fix comparison in tests/run-emitter.c
* yaml/libyaml#117 -- Fix typo error
* yaml/libyaml#119 -- The closing single quote needs to be indented...
* yaml/libyaml#121 -- fix token name typos in comments
* yaml/libyaml#122 -- Revert removing of open_ended after top level plain scalar
* yaml/libyaml#125 -- Cherry-picks from PR 27
* yaml/libyaml#135 -- Windows/C89 compatibility
* yaml/libyaml#136 -- allow override of Windows static lib name


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@498674 35697150-7ecd-e111-bb59-0022644237b5
uqs pushed a commit to freebsd/freebsd-ports that referenced this pull request Apr 11, 2019
Fix portlint errors in Makefile

Changes in 0.2.2:
* yaml/libyaml#95 -- build: do not install config.h
* yaml/libyaml#97 -- appveyor.yml: fix Release build
* yaml/libyaml#103 -- Remove unused code in yaml_document_delete
* yaml/libyaml#104 -- Allow colons in plain scalars inside flow collections
* yaml/libyaml#109 -- Fix comparison in tests/run-emitter.c
* yaml/libyaml#117 -- Fix typo error
* yaml/libyaml#119 -- The closing single quote needs to be indented...
* yaml/libyaml#121 -- fix token name typos in comments
* yaml/libyaml#122 -- Revert removing of open_ended after top level plain scalar
* yaml/libyaml#125 -- Cherry-picks from PR 27
* yaml/libyaml#135 -- Windows/C89 compatibility
* yaml/libyaml#136 -- allow override of Windows static lib name


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@498674 35697150-7ecd-e111-bb59-0022644237b5
uqs pushed a commit to freebsd/freebsd-ports that referenced this pull request Apr 11, 2019
Fix portlint errors in Makefile

Changes in 0.2.2:
* yaml/libyaml#95 -- build: do not install config.h
* yaml/libyaml#97 -- appveyor.yml: fix Release build
* yaml/libyaml#103 -- Remove unused code in yaml_document_delete
* yaml/libyaml#104 -- Allow colons in plain scalars inside flow collections
* yaml/libyaml#109 -- Fix comparison in tests/run-emitter.c
* yaml/libyaml#117 -- Fix typo error
* yaml/libyaml#119 -- The closing single quote needs to be indented...
* yaml/libyaml#121 -- fix token name typos in comments
* yaml/libyaml#122 -- Revert removing of open_ended after top level plain scalar
* yaml/libyaml#125 -- Cherry-picks from PR 27
* yaml/libyaml#135 -- Windows/C89 compatibility
* yaml/libyaml#136 -- allow override of Windows static lib name
kwm81 pushed a commit to freebsd/freebsd-ports-gnome that referenced this pull request Apr 13, 2019
Fix portlint errors in Makefile

Changes in 0.2.2:
* yaml/libyaml#95 -- build: do not install config.h
* yaml/libyaml#97 -- appveyor.yml: fix Release build
* yaml/libyaml#103 -- Remove unused code in yaml_document_delete
* yaml/libyaml#104 -- Allow colons in plain scalars inside flow collections
* yaml/libyaml#109 -- Fix comparison in tests/run-emitter.c
* yaml/libyaml#117 -- Fix typo error
* yaml/libyaml#119 -- The closing single quote needs to be indented...
* yaml/libyaml#121 -- fix token name typos in comments
* yaml/libyaml#122 -- Revert removing of open_ended after top level plain scalar
* yaml/libyaml#125 -- Cherry-picks from PR 27
* yaml/libyaml#135 -- Windows/C89 compatibility
* yaml/libyaml#136 -- allow override of Windows static lib name
swills pushed a commit to swills/freebsd-ports that referenced this pull request Apr 14, 2019
Fix portlint errors in Makefile

Changes in 0.2.2:
* yaml/libyaml#95 -- build: do not install config.h
* yaml/libyaml#97 -- appveyor.yml: fix Release build
* yaml/libyaml#103 -- Remove unused code in yaml_document_delete
* yaml/libyaml#104 -- Allow colons in plain scalars inside flow collections
* yaml/libyaml#109 -- Fix comparison in tests/run-emitter.c
* yaml/libyaml#117 -- Fix typo error
* yaml/libyaml#119 -- The closing single quote needs to be indented...
* yaml/libyaml#121 -- fix token name typos in comments
* yaml/libyaml#122 -- Revert removing of open_ended after top level plain scalar
* yaml/libyaml#125 -- Cherry-picks from PR 27
* yaml/libyaml#135 -- Windows/C89 compatibility
* yaml/libyaml#136 -- allow override of Windows static lib name


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@498674 35697150-7ecd-e111-bb59-0022644237b5
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 19, 2019
Changes
=======

* yaml/libyaml#95 -- build: do not install config.h
* yaml/libyaml#97 -- appveyor.yml: fix Release build
* yaml/libyaml#103 -- Remove unused code in yaml_document_delete
* yaml/libyaml#104 -- Allow colons in plain scalars inside flow collections
* yaml/libyaml#109 -- Fix comparison in tests/run-emitter.c
* yaml/libyaml#117 -- Fix typo error
* yaml/libyaml#119 -- The closing single quote needs to be indented...
* yaml/libyaml#121 -- fix token name typos in comments
* yaml/libyaml#122 -- Revert removing of open_ended after top level plain scalar
* yaml/libyaml#125 -- Cherry-picks from PR 27
* yaml/libyaml#135 -- Windows/C89 compatibility
* yaml/libyaml#136 -- allow override of Windows static lib name
perlpunk added a commit that referenced this pull request Dec 26, 2019
In YAML 1.1, the document end marker `...` is optional even if the next document starts with a directive:
https://github.com/yaml/pyyaml/blob/master/tests/data/spec-07-09.canonical
```
%YAML 1.1
---
!!str "foo"
%YAML 1.1
---
!!str "bar"
%YAML 1.1
---
!!str "baz"
```
It is only required if the scalar is "open ended", for example for plain scalars.

In YAML 1.2 the `...` marker is always required before a directive.

My suggestion would be to make the output 1.2 compatible. It will still be 1.1 compatible, so that shouldn't be a problem.

I believe this will also make it easier to fix #123 which was introduced with the last fixes regarding `open_ended`. I think I can make a fix for this soon after this issue is fixed.
Fixing #123 without this would be a bit more complicated.

If we do this, we also need to adjust PyYAML to behave the same.

Related issues/commits:
- #60
- #122
- 56400d9, 8ee83c0, 56f4b17
perlpunk added a commit that referenced this pull request Mar 23, 2020
In YAML 1.1, the document end marker `...` is optional even if the next document starts with a directive:
https://github.com/yaml/pyyaml/blob/master/tests/data/spec-07-09.canonical
```
%YAML 1.1
---
!!str "foo"
%YAML 1.1
---
!!str "bar"
%YAML 1.1
---
!!str "baz"
```
It is only required if the scalar is "open ended", for example for plain scalars.

In YAML 1.2 the `...` marker is always required before a directive.

My suggestion would be to make the output 1.2 compatible. It will still be 1.1 compatible, so that shouldn't be a problem.

I believe this will also make it easier to fix #123 which was introduced with the last fixes regarding `open_ended`. I think I can make a fix for this soon after this issue is fixed.
Fixing #123 without this would be a bit more complicated.

If we do this, we also need to adjust PyYAML to behave the same.

Related issues/commits:
- #60
- #122
- 56400d9, 8ee83c0, 56f4b17
perlpunk added a commit that referenced this pull request Mar 26, 2020
In YAML 1.1, the document end marker `...` is optional even if the next document starts with a directive:
https://github.com/yaml/pyyaml/blob/master/tests/data/spec-07-09.canonical
```
%YAML 1.1
---
!!str "foo"
%YAML 1.1
---
!!str "bar"
%YAML 1.1
---
!!str "baz"
```
It is only required if the scalar is "open ended", for example for plain scalars.

In YAML 1.2 the `...` marker is always required before a directive.

My suggestion would be to make the output 1.2 compatible. It will still be 1.1 compatible, so that shouldn't be a problem.

I believe this will also make it easier to fix #123 which was introduced with the last fixes regarding `open_ended`. I think I can make a fix for this soon after this issue is fixed.
Fixing #123 without this would be a bit more complicated.

If we do this, we also need to adjust PyYAML to behave the same.

Related issues/commits:
- #60
- #122
- 56400d9, 8ee83c0, 56f4b17
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.

4 participants