Skip to content

Conversation

@notatallshaw
Copy link
Member

I don't know if this is a good idea, and will close if anyone objects.

The motivation is to fix the fact that pip's .pre-commit-config.yaml isn't a valid yaml file (bourumir-wyngs/serde-saphyr#26 (comment) / j178/prek#1111).

I couldn't find a yaml linter via pre-commit that was strict enough to pick this up as an issue. So instead I picked a popular yaml formatter that formats the yaml file to be compliant. I picked the smallest number of settings which caused the smallest diff: https://github.com/google/yamlfmt/blob/main/docs/config-file.md

I can just manually fix .pre-commit-config.yaml, but there's no validation from our tooling.

@notatallshaw notatallshaw added the skip news Does not need a NEWS file entry (eg: trivial changes) label Nov 20, 2025
@ichard26
Copy link
Member

ichard26 commented Nov 20, 2025

Given the file is only used by people developing pip (aka us), what benefit do we gain from enforcing proper YAML syntax? I'm not opposed to fixing our yaml files, but I'm also not a fan of code churn for no good reason.

Are you looking into pre-commit alternatives that expect spec-compliant YAML files?

@notatallshaw
Copy link
Member Author

Are you looking into pre-commit alternatives that expect spec-compliant YAML files?

Yeah, locally I use prek, not pre-commit, and it was unable to use a spec-compliant YAML parser, with pip's codebase being the primary example.

I would like a stricter yaml linter, but the next best thing appeared to be a formatter, so enforce this required closing square bracket indentation. Otherwise I'll just fix it, and hopefully it will rain fixed.

@ichard26
Copy link
Member

Honestly, I'll say that if this blocks or materially impacts your ability to get work done, just merge this. I doubt anyone cares particularly for the formatting of our .pre-commit-config.yaml (and the git blame is not needed). cc @pfmoore

- { number: 1, pytest-filter: "not test_install" }
- { number: 2, pytest-filter: "test_install" }
- {number: 1, pytest-filter: "not test_install"}
- {number: 2, pytest-filter: "test_install",}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird. Why does one of these have a final comma but not the other?

Copy link
Member Author

@notatallshaw notatallshaw Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this is weird, I'll dig into why this is happening.

- repo: https://github.com/psf/black-pre-commit-mirror
rev: 25.9.0
hooks:
- id: black
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this changed the order of the blocks? If we need to keep a specific order, that seems a bit more annoying than just fixing formatting.

Copy link
Member Author

@notatallshaw notatallshaw Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only order that changed is that the yaml linting takes place after the yaml formatting.

This is a general best practice when both formatting and linting, if they both find issues then if the lint occurs before the format the format will change what the line numbers the lint was referring to, but if the format occurs before the lint then the lint line numbers remain valid.

Unfortunately the git diff didn't do a good job on matching that things are mostly the same.

@pfmoore
Copy link
Member

pfmoore commented Nov 20, 2025

I’ve added a couple of comments, but basically I’m in agreement with @ichard26 - it’s not a big deal, and if it helps you I don’t object.

@hugovk
Copy link
Contributor

hugovk commented Nov 21, 2025

The motivation is to fix the fact that pip's .pre-commit-config.yaml isn't a valid yaml file (bourumir-wyngs/serde-saphyr#26 (comment) / j178/prek#1111).

btw prek reverted that stricter YAML parser (j178/prek#1112) and prek 0.2.17 can handle the current .pre-commit-config.yaml again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip news Does not need a NEWS file entry (eg: trivial changes)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants