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

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

Open
sylvainfaivre opened this issue Feb 7, 2024 · 2 comments
Labels
Type: Bug Something isn't working

Comments

@sylvainfaivre
Copy link

See saltstack/salt#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 sylvainfaivre added the Type: Bug Something isn't working label Feb 7, 2024
@ohartl
Copy link

ohartl commented Apr 15, 2024

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

@NoRePercussions
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants