adrienverge / yamllint

A linter for YAML files.
GNU General Public License v3.0
2.9k stars 278 forks source link

Block specific, error prone, yaml key names #694

Open AvnerCohen opened 1 month ago

AvnerCohen commented 1 month ago
null:
  code: XK
  value1: true
  value2: true

This "null" is valid, but will fail in many cases, depends on the programming language reading this yaml.

Is there any yamllint rule I could use to protect such "error prone key names" ? (I can think of none, false and true as additional candidates)

adrienverge commented 1 month ago

Hello Avner,

Indeed, although confusing in most contexts, this code is valid YAML!

Yamllint doesn't currently provide a way to forbid specific types of keys. We could imagine a new rule like key-types like this:

rules:
  key-types:
    # types from https://perlpunk.github.io/YAML-PP-p5/schemas.html
    forbid-null: true|false  # tag:[yaml.org](http://yaml.org/),2002:null
    forbid-bool: true|false  # tag:[yaml.org](http://yaml.org/),2002:bool
    forbid-int: true|false   # tag:[yaml.org](http://yaml.org/),2002:int
    forbid-float: true|false # tag:[yaml.org](http://yaml.org/),2002:float
    forbid-str: true|false   # tag:[yaml.org](http://yaml.org/),2002:str
AvnerCohen commented 1 month ago

This would be amazing! I am not at familiar with the YAML vocabulary, I am wondering if the fact the "forbid" here, should be explicitly to "key names" ? (or should it be "tag names") ? and so the rule would be:

rules:
  key-types:
    forbid-as-key-null: true|false

etc..

But this is cosmetic, and I suspect you know what you are doing :)

Would be amazing to see something like that implemented.

Also, important to consider case here (Null, None, null, NULL etc).

adrienverge commented 1 month ago

I am not at familiar with the YAML vocabulary, I am wondering if the fact the "forbid" here, should be explicitly to "key names" ? (or should it be "tag names") ?

I'm not sure, because this rule would be about forbidding the use of specific types (null, boolean, integers, for instance values null, true or 1.2e3), not about names (e.g. "null" or "string"). (Maybe I didn't understand your question well, in which case feel free to rephrase.)

Also, important to consider case here (Null, None, null, NULL etc).

I think in YAML 1.2 only the following values are null: null, Null, NULL and ~ (my source is @perlpunk's https://perlpunk.github.io/YAML-PP-p5/schemas.html).

Would be amazing to see something like that implemented.

Contributions are welcome :) They need to follow guidelines and come with tests and documentation.

AvnerCohen commented 1 month ago

Thanks @adrienverge So, to be sure I get your suggestion here:

dict_of_values:
   null: 'value'

In the above example, if I get this right, "null" is used as "Key" in the dict_of_values dictionary.

So I think I am asking about how to block "keys" with certain and specific "string values" that might be ok in YAML, but as soon as you are being "parsed" by python/javascript/etc the name chosen might cause issues down the line..

adrienverge commented 4 weeks ago

Hello Avner,

I'm not sure I get you right, maybe there's a misunderstanding about the type of null in YAML?

In your example YAML snippet, null is not a string, it the null value. Hence, it's not a key with a name, it's a reserved keyword in YAML. Said differently, the 2 following dictionaries are different:

# Example 1: null value
dict_of_values:
   null: 'value'
# Example 2: string value, with value "null"
dict_of_values:
   'null': 'value'

Unlike JSON, using the null value as a key in a mapping is allowed in YAML (see examples at the end of https://yaml.org/type/null.html).

If your proposal is to create a rule to avoid the use of dict: {null: 'value'}, I find it meaningful. (And we could allow forbidding similar cases like dict: {4: 'value'} or dict: {false: 'value'}).

However if it's about forbidding specific key names (= string values like dict: {'null': 'value'}) I'm against it, because yamllint doesn't aim to check the values / content of YAML data (only its form).

AvnerCohen commented 4 weeks ago

Thanks @adrienverge thanks for sticking with me on this :) I imagine this is a combination of my language barrier (yaml) and my language barrier (not native english).. But I feel we are making progress :)

We are aligned on the goal here, it's on "null value" case. To illustrate the issue I am seeing:

>>> import yaml
>>> yaml_string = """
... dict_of_values:
...   null: 'value'
... """
>>> yaml.safe_load(yaml_string)
{'dict_of_values': {None: 'value'}}
>>>

The resulting "None" is going to cause issues down the road and also creates interoperability issues because here is how it looks in a Javascript Yaml parser:

{
  "dict_of_values": {
    "null": "value"
  }
}

Going back to your original suggestion:

rules:
  key-types:
    # types from https://perlpunk.github.io/YAML-PP-p5/schemas.html
    forbid-null: true|false  # tag:[yaml.org](http://yaml.org/),2002:null
    forbid-bool: true|false  # tag:[yaml.org](http://yaml.org/),2002:bool
    forbid-int: true|false   # tag:[yaml.org](http://yaml.org/),2002:int
    forbid-float: true|false # tag:[yaml.org](http://yaml.org/),2002:float
    forbid-str: true|false   # tag:[yaml.org](http://yaml.org/),2002:str

Are you able to provide any pointers on how to progress this with a contribution ?

adrienverge commented 4 weeks ago

To illustrate the issue I am seeing: …

{'dict_of_values': {None: 'value'}}

Agreed :+1: then we're talking about the same thing :slightly_smiling_face:

Are you able to provide any pointers on how to progress this with a contribution ?

I suggest checking out CONTRIBUTING.rst first, then for a good start you can look at the code of other rules, in particular read some commits that introduced new rules into yamllint (you'll see they must come with tests and documentation). Good luck!

AvnerCohen commented 4 weeks ago

@adrienverge Please see the initial suggested direction at https://github.com/adrienverge/yamllint/pull/695 There is more way to go of course, but wanted to see if this make sense as the general direction or I am completely off, which is not impossible..:)

AvnerCohen commented 2 weeks ago

@adrienverge wondering if you had a chance to review this direction ? #695 Wanted to be sure, before I spend more time optimizing it and adding docs.

adrienverge commented 2 weeks ago

Hello Avner,

I'm willing to review and release ready-to-merge PRs on my free time, but at the moment I don't have the bandwidth to implement nor guide implementation of development requests.

At first glance I see that #695 does not respect CONTRIBUTING.rst, and does not implement what we talked about (it creates 4 new rules instead of 1 named key-types).

AvnerCohen commented 1 week ago

Thanks @adrienverge , I totally understand and appreciate your input. I'll ping when I feel there is something complete for a review.