salt-lint icon indicating copy to clipboard operation
salt-lint copied to clipboard

207, 208 and 210 are useless, as salt removes leading zeroes when parsing integers with yaml

Open sylvainfaivre opened this issue 1 year ago • 2 comments

See https://github.com/saltstack/salt/issues/60773 and https://github.com/saltstack/salt/blob/2b364c92e6319ec3a9884afff10e6e4e1e1642db/salt/utils/yamlloader.py#L89

When loading a yaml scalar, salt takes special care of removing the leading zero unless it's starting with 0b or 0x.

This has been the case for more than 10 years, see https://github.com/saltstack/salt/blame/bbdcd5d4b496845eb6a6811b7c04898a99b4e11e/salt/utils/yamlloader.py#L91C53-L91C55

So I would argue that these rules are useless and could be removed. Or at the least, if people want to use them to ensure coding style consistency, add a warning in the docs explaining they are not real issues.

I hope this report gets the conversation going. Should we reach a consensus on this, I'd be willing to propose a PR.

sylvainfaivre avatar Feb 07 '24 13:02 sylvainfaivre

Correct, it seems like this was useful when we still used python2, but nowadays this is not useful for rulesets / best practices anymore.

ohartl avatar Apr 15 '24 12:04 ohartl

Since Salt no longer complies to any particular YAML version (the custom loader does not recognize old-style octal 0123 nor new-style octal 0o123, I'd suggest we stay with the least common denominator of YAML parsers and keep these rules.

For another example of a major library that has done this, https://github.com/go-yaml/yaml continues to support old-style, citing "most parsers still use the old format".

I'd also pose that a file mode is an octal number, and treating it as decimal to begin with is a hack for convenience. I don't think it helps to allow any ambiguity as to whether a file mode will be parsed as octal or decimal, and whether that parsing will be supported, so I believe that enforcing a string format for the mode is sensible regardless upstream support, if salt-lint is aiming to be opinionated.

NoRePercussions avatar Jul 03 '24 04:07 NoRePercussions