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

Test NTY5 (Missing space in YAML directive) should not be an error #38

Closed
eemeli opened this issue Jul 1, 2018 · 3 comments
Closed

Comments

@eemeli
Copy link
Member

eemeli commented Jul 1, 2018

The test was introduced a few weeks ago in 30f372c. The relevant directives are defined with:

[82]           l-directive ::= "%"
                               ( ns-yaml-directive | ns-tag-directive | ns-reserved-directive )
                               s-l-comments
[83] ns-reserved-directive ::= ns-directive-name
                               ( s-separate-in-line ns-directive-parameter )*
[84]     ns-directive-name ::= ns-char+
[86]     ns-yaml-directive ::= "Y" "A" "M" "L" s-separate-in-line ns-yaml-version
[66]    s-separate-in-line ::= s-white+ | /* Start of line */

Therefore, a directive line %YAML1.2 should get parsed as ns-reserved-directive, regarding which the following instruction applies: "A YAML processor should ignore unknown directives with an appropriate warning."

Effectively, as "appropriate warning" is rather implementation-specific, it might be best to just remove this test completely. Or to just leave out the error, as parsers are expected to deal with this situation without an error.

@hvr
Copy link
Collaborator

hvr commented Jul 1, 2018

For the record the NTY5 test was added as consequence of #34

But I think I agree that a non-greedy application of rule 82 means that a mistyped YAML (or TAG) directive ought to fallback to the ns-reserved-directive production.

eemeli added a commit to eemeli/yaml-test-suite that referenced this issue Jul 1, 2018
@perlpunk
Copy link
Member

perlpunk commented Jul 4, 2018

Yeah, I was thinking about that too. I think we should remove that test...

eemeli added a commit to eemeli/yaml-test-suite that referenced this issue Jul 8, 2018
@perlpunk perlpunk added this to Backlog in Release 2020-02-11 Dec 27, 2019
perlpunk added a commit that referenced this issue Jan 18, 2020
@perlpunk perlpunk mentioned this issue Jan 18, 2020
@perlpunk perlpunk moved this from Backlog to In Progress in Release 2020-02-11 Jan 18, 2020
perlpunk added a commit that referenced this issue Jan 18, 2020
@perlpunk perlpunk moved this from In Progress to Done in Release 2020-02-11 Jan 18, 2020
@perlpunk
Copy link
Member

Removed the test, created new release 2020-02-11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

3 participants