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

Extend yamllint hook #398

Merged
merged 6 commits into from
Apr 2, 2024
Merged

Conversation

totoroot
Copy link
Collaborator

I added a couple of options to the yamllint hook, as so far it was only possible to pass a path to the configuration file.
In some comments, I also reasoned why some options weren't added as they do not make sense for a pre-commit hook.

Similar to how I did it for the typos hook, there is now also an option to pass a multiline string as configuration to yamllint. Additionally yamllint allows for a serialized configuration, but the multiline string configuration option is the preferred way. Of course just providing a path to a configuration file remains possible.

The behaviour of the hook itself mostly stayed the same, although the relaxed option was removed in favour of a more generalized preset option, that allows us to easily add presets, should more ever be added to yamllint.

A pre-commit hook only makes sense, if it prevents you from committing if errors were found, so --strict should definitely be the default.

Breaking changes:

  • settings.yamllint.relaxed = true; is now settings.yamllint.preset = "relaxed";
  • settings.yamllint.strict was added and defaults to true ➡️ strict error checking for hook

Additional changes:

For some hooks I recently worked on, I noticed that I was shadowing the top-level attribute config, which is never a good idea, so I changed the multiline configuration options to configuration.
Affected hooks are typos and vale. The options were only added recently.

@totoroot totoroot force-pushed the extend-yamllint-hook branch from eb418b0 to 206bc45 Compare February 16, 2024 11:07
@totoroot totoroot requested a review from domenkozar February 16, 2024 12:15
@totoroot totoroot self-assigned this Feb 16, 2024
@domenkozar
Copy link
Member

Sorry for the mess, we revamed module structure in #397, could you port these changes over?

@totoroot totoroot force-pushed the extend-yamllint-hook branch 3 times, most recently from d4a9d76 to 20fbe2c Compare March 19, 2024 10:32
@domenkozar
Copy link
Member

error: evaluation aborted with the following error message: 'cannot find attribute `hooks.yamllint.settings.relaxed''

@totoroot
Copy link
Collaborator Author

totoroot commented Mar 21, 2024

> error: evaluation aborted with the following error message: 'cannot find attribute `hooks.yamllint.settings.relaxed''

@domenkozar Please see my reply above. I don't understand why this error comes up. I thought I used mkRenamedOptionModule correctly but apparently not. See 2602c35.

@domenkozar I believe I fixed all warnings and rebased onto master again. Ready for review/merge.

@totoroot totoroot force-pushed the extend-yamllint-hook branch from 2602c35 to bf4ca5f Compare March 29, 2024 13:22
@totoroot totoroot force-pushed the extend-yamllint-hook branch from bf4ca5f to 34fdf24 Compare March 29, 2024 13:54
@domenkozar
Copy link
Member

Ci is failing

@domenkozar domenkozar merged commit e35aed5 into cachix:master Apr 2, 2024
4 checks passed
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.

2 participants