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

truthy: Adapt forbidden values based on YAML version #650

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

adrienverge
Copy link
Owner

@adrienverge adrienverge commented Jan 31, 2024

Specification of YAML ≤ 1.1 has 22 boolean values:

y     | Y            | n     | N
yes   | Yes   | YES  | no    | No    | NO
true  | True  | TRUE | false | False | FALSE
on    | On    | ON   | off   | Off   | OFF

Whereas YAML 1.2 spec recognizes only 6 1:

true  | True  | TRUE | false | False | FALSE

For documents that explicit state their YAML spec version at the top of the document, let's adapt the list of forbidden values.

In the future, we should:

  • implement a configuration option to declare the default YAML spec version, e.g. default-yaml-spec-version: 1.2,
  • consider making 1.2 the default in a future release (this would be a slight breaking change, but yamllint always tried to be 1.2-compatible).
  • consider adapting yamllint to other 1.1 vs. 1.2 differences 2.

Solves: #587

Related to: #559 #540 #430 #344 #247 #232 #158

Footnotes

  1. https://yaml.org/spec/1.2.2/#1032-tag-resolution

  2. https://yaml.org/spec/1.2.2/ext/changes/#changes-in-version-12-revision-120-2009-07-21

@coveralls
Copy link

coveralls commented Jan 31, 2024

Coverage Status

coverage: 99.823% (+0.001%) from 99.822%
when pulling 3b6a3df on feat/truthy-depending-on-yaml-spec-version
into 9931ad6 on master.

@adrienverge
Copy link
Owner Author

@perlpunk today I attended a great talk in Brussels which said there were 22 values (not 18), so I updated the commit message:

-Specification of YAML ≤ 1.1 has 18 boolean values:
+Specification of YAML ≤ 1.1 has 22 boolean values:

Notes:
- I haven't adapted the yamllint rule code, because PyYAML treats the 4 extra values y, Y, n and N as strings (so it's neither 1.1- nor 1.2-complete).
- I had a hard time finding the exact list on yaml.org (it's not directly on the YAML 1.1 specification page).

fosdem2024

By the way, I read that between 1.1 and 1.2 "the merge << and value = special mapping keys have been removed" but I heard that some 1.2-compatible YAML parsing libs implement the merge marker << as an option. Do you think yamllint should complain when it finds both a tag %YAML 1.2 and a merge marker <<?

@perlpunk
Copy link

perlpunk commented Feb 5, 2024

@adrienverge thanks very much!
Also, nice to meet you :)

Yeah, I think the non-standard behaviour of PyYAML makes it a bit more difficult.
And I think more libraries will implement things like << optionally. I think it's a good idea because << is well known and very useful. Of course it makes our job harder...

Regarding %YAML 1.2, I'm not sure how often it will be used in the future. It's meant for that kind of thing, but I think people might avoid it because they don't want to type it in every YAML file.

For my YAML::PP library I made an option schema, which is an array, so you can say schema => ['Core', 'Merge']) for example, so I was thinking to do something similar for yamltidy. But while Core stands for a set of tags, Merge is just one tag. In YAML::PP, both are a class, e.g. YAML::PP::Schema::Merge, that's why it works. For getting the PyYAML behaviour I would probably just add another class YAML::PP::Schema::PyYAML1_1.

So I don't have a clear idea here yet how a linter/formatter config option could look like...

The so called tag repository is kind of hidden, yeah. You can find it in the 1.1 spec under YAML tag repository.
YAML 1.1 was always a draft actually, and then for 1.2 it wasn't used anymore.
So then I put together this page.
The data in that repository can be used as the official test suite for resolving tags.
I hope we can move that into yaml-test-suite at some point.

@perlpunk
Copy link

perlpunk commented Feb 5, 2024

...and I just realized that I forgot No in the test data (I just have NO and no), that's why https://perlpunk.github.io/yaml-test-schema/data.html only shows 21 booleans!

Specification of YAML ≤ 1.1 has 22 boolean values:

    y     | Y            | n     | N
    yes   | Yes   | YES  | no    | No    | NO
    true  | True  | TRUE | false | False | FALSE
    on    | On    | ON   | off   | Off   | OFF

Whereas YAML 1.2 spec recognizes only 6 [^1]:

    true  | True  | TRUE | false | False | FALSE

For documents that explicit state their YAML spec version at the top of
the document, let's adapt the list of forbidden values.

In the future, we should:
- implement a configuration option to declare the default YAML spec
  version, e.g. `default-yaml-spec-version: 1.2`,
- consider making 1.2 the default in a future release (this would be a
  slight breaking change, but yamllint always tried to be
  1.2-compatible).
- consider adapting yamllint to other 1.1 vs. 1.2 differences [^2].

Solves: #587

Related to: #559 #540 #430 #344 #247 #232 #158

[^1]: https://yaml.org/spec/1.2.2/#1032-tag-resolution
[^2]: https://yaml.org/spec/1.2.2/ext/changes/#changes-in-version-12-revision-120-2009-07-21
@adrienverge adrienverge changed the title truthy: Adapt valid values based on YAML version truthy: Adapt forbidden values based on YAML version Feb 6, 2024
@adrienverge
Copy link
Owner Author

@perlpunk thank you! I'll bookmark https://perlpunk.github.io/yaml-test-schema/schemas.html as it's very handy (and I'm glad you could detect a missing entry in the test list thanks to my message :))

I agree that prepending explicit tags like %YAML 1.2 is not practical and few users would do it. I'm attracted by the idea of making yamllint 1.2-compatible by default, but that would partially defeat the purpose of some rules (e.g. octal-values or truthy which help detecting values valid in 1.1 but not in 1.2).

Anyway, I believe the change of this pull request is a good move so I'll merge it.
And let's not lint << + YAML 1.2 cases since they are used in practice and quite useful.

@adrienverge adrienverge force-pushed the feat/truthy-depending-on-yaml-spec-version branch from 0765b0a to 3b6a3df Compare February 6, 2024 08:43
@adrienverge adrienverge merged commit 01df5bf into master Feb 6, 2024
11 of 14 checks passed
@adrienverge adrienverge deleted the feat/truthy-depending-on-yaml-spec-version branch February 6, 2024 08:45
@perlpunk
Copy link

perlpunk commented Feb 6, 2024

@perlpunk thank you! I'll bookmark https://perlpunk.github.io/yaml-test-schema/schemas.html

Also, like I said, you can use the data https://github.com/perlpunk/yaml-test-schema/tree/master/data for your test suite.
I will add the No thing ASAP :)

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.

3 participants