chore: Integrate yamllint into the project
#747
Conversation
|
@yermulnik this is the yamllint config I was talking about earlier. |
| - file | ||
| - yaml | ||
| args: | ||
| - --strict |
There was a problem hiding this comment.
This being a CLI argument is an exception and is here only because yamllint does not allow configuring it in the common config: adrienverge/yamllint#601.
webknjaz
force-pushed
the
maintenance/yamllint
branch
from
ac1b1a4 to
b081c79
Compare
|
Looks like the CI is having problems accessing DockerHub. It's not related to this PR and somebody should restart the CI jobs once GH fixes their platform issues. |
| - repo: https://github.com/adrienverge/yamllint.git | ||
| rev: v1.35.1 | ||
| hooks: | ||
| - id: yamllint |
There was a problem hiding this comment.
It not format yaml locally, that's is unfortunate.
Better use
- repo: https://github.com/jumanjihouse/pre-commit-hook-yamlfmt
rev: 0.2.3
hooks:
- id: yamlfmt
args: [
--mapping, '2',
--sequence, '4',
--offset, '2',
--width, '100',
--implicit_start,
--preserve_null,
]which actually format code, and enforce style which should be used here anyway
There was a problem hiding this comment.
They actually can be combined by
- repo: https://github.com/adrienverge/yamllint
rev: v1.35.1
hooks:
- id: yamllint
types:
- file
- yaml
args: [--format, parsable, --strict]
- repo: https://github.com/jumanjihouse/pre-commit-hook-yamlfmt
rev: 0.2.3
hooks:
- id: yamlfmt
Just need to change .yamllint a bit
Nwm, yamlfmt don't uses .yamllint
There was a problem hiding this comment.
This is a linter, not a formatter. I'm yet to see a formatter working well. But FWIW this is out of the scope here. This PR is about a specific linter and anything else is an undesired scope creep that can be debated elsewhere.
There was a problem hiding this comment.
They actually can be combined by
This is a bad idea as formatters should always go before linters to prevent double errors.
There was a problem hiding this comment.
In this case, yamllint is redundant as yamlfmt will fix all these issues
If you want to have it for some unknown to me reason, I can adjust yamllint config
There just only issue which I see - such type of comment are not handled properly by yamlfmt, but they always can be moved or above, or just get rid of >- in simple constructions
There was a problem hiding this comment.
Formatting is out of the scope. So that's irrelevant. But to answer the question about a separate line — no, it's not normally okay to put comments on a separate line due to context drifting. This is important in any code-like files, including but not limited to YAML. The comment is there intentionally and must not be moved.
Just to reiterate, the scope of this PR is solely adding one specific linter and nothing else. Any other attempts to change the scope indefinitely are to be rejected with prejudice.
There was a problem hiding this comment.
If you want to limit just to linter, here we go then.
| indentation: | ||
| level: error | ||
| indent-sequences: false |
There was a problem hiding this comment.
| indentation: | |
| level: error | |
| indent-sequences: false | |
| indentation: | |
| level: error | |
| indent-sequences: true | |
| line-length: | |
| max: 100 | |
| document-start: | |
| present: false | |
| comments: | |
| min-spaces-from-content: 1 |
There was a problem hiding this comment.
I think the line length setting is unreasonably long. Also, double whitespace before a comment is an accepted convention. A single space seems dumb with this being required just about everywhere.
Unnecessary sequence indentation would just make the lines longer, forcing the increase of other rule defaults to ridiculous values.
In general, this suggestion looks like a snowball of bad changes.
Additionally, it's going to trigger more reformatting that is not in the scope right now. I don't want things to diverge from other projects just because of a silly nitpick. You can make the style less readable yourself, once I'm done sending patches.
There was a problem hiding this comment.
These "Unnecessary sequence indentation" exist by the same reason why Python have 4 space indentation, not 2 - it less error-prone.
It is much simpler to figure out where part args belong to in next construction
- repo: https://github.com/adrienverge/yamllint.git
rev: v1.35.1
hooks:
- id: yamllint
types:
- file
- yaml
args:
- --strictthan in
- repo: https://github.com/adrienverge/yamllint.git
rev: v1.35.1
hooks:
- id: yamllint
types:
- file
- yaml
args:
- --strictjust because you can't have list and K/V on the same level.
This is a common and very annoying problem, and the right solution to where indent should be is not covered by linters or formatters, so the only way to deal with it if there is no such indents and no jsonschema check - manually go and check documentation each time for tool or run this tool.
And just noticed "Unnecessary sequence indentation" - is default setting of yamllint itself, and YAML formatter built-in in VS Code which, btw, ignores .yamllint config, but respects .editorconfig
More complex solution to it could be indentation.spaces=4, but it looks much worse
There was a problem hiding this comment.
My bad, there are no built-in VS Code YAML formatter, it's provided by redhat.vscode-yaml extension.
There was a problem hiding this comment.
I think the line length setting is unreasonably long
Second vertical line is 80 chars, third - 100 chars
You added 18 ignores for line-length in such simple project. And after that you said that it "unreasonably long"?
There was a problem hiding this comment.
About
document-start:
present: false
comments:
min-spaces-from-content: 1First is just my neat-peak to skip unnecessary syntax, second - how yamlfmt formats comments and that, sadly not configurable, but I love to have automation which able to maintain "good enough" style, than go and fix issues manually to the best possible style.
But yes, they both can be skipped for now
| - repo: https://github.com/adrienverge/yamllint.git | ||
| rev: v1.35.1 | ||
| hooks: | ||
| - id: yamllint |
There was a problem hiding this comment.
If you want to limit just to linter, here we go then.
MaxymVlasov
changed the title
yamllint into the projectyamllint into the project
The initial config does not change many defaults and mostly relies on the upstream config. The only changes are to the sequence style, quoting and allowed truthy values that accomodate for GHA.
webknjaz
force-pushed
the
maintenance/yamllint
branch
from
c82f383 to
f2ead2d
Compare
|
@yermulnik could you restart the cancelled job? |
The initial config does not change many defaults and mostly relies on the upstream config. The only changes are to the sequence style, quoting and allowed truthy values that accomodate for GHA.
Put an
xinto the box if that apply:Description of your changes
N/A
How can we test changes
..stare at the diff?
tox r -qq -e pre-commit -- yamllint --all-filesotherwise.