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

Add pre-commit hook to run config verify #4602

Merged

Conversation

matthewhughes934
Copy link
Contributor

Add this as a way to easily ensure configs stay valid. Filenames are not passed to the hook, this means For the default case (config in one of the default searched files), the config is picked up automatically and there's nothing extra to configure. For the case of a non-standard config setup the hook can be configured like

repos:
-   repo: https://github.com/golangci/golangci-lint
    rev: v1.57.2
    hooks:
    -   id: golangci-lint-config-verify
        # config is kept at '.golangci-config.yaml'
        files: \.golangci-config\.yaml
        args: ['--config', '.golangci-config.yaml']

Copy link

boring-cyborg bot commented Mar 30, 2024

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Mar 30, 2024

CLA assistant check
All committers have signed the CLA.

Add this as a way to easily ensure configs stay valid. Filenames are not
passed to the hook, this means For the default case (config in one of
the default searched files), the config is picked up automatically and
there's nothing extra to configure. For the case of a non-standard
config setup the hook can be configured like

    repos:
    -   repo: https://github.com/golangci/golangci-lint
        rev: v1.57.2
        hooks:
        -   id: golangci-lint-config-verify
            # config is kept at '.golangci-config.yaml'
            files: \.golangci-config\.yaml
            args: ['--config', '.golangci-config.yaml']
@matthewhughes934 matthewhughes934 force-pushed the add-config-verify-pre-commit-hook branch from 0c67d00 to f13fbbd Compare March 30, 2024 20:49
@ldez ldez self-requested a review March 30, 2024 20:52
@ldez ldez added blocked Need's direct action from maintainer area: pre-commit and removed blocked Need's direct action from maintainer labels Mar 30, 2024
@matthewhughes934
Copy link
Contributor Author

I skipped submitting a feature request issue since it seemed as simple to just discuss the change itself

@matthewhughes934 matthewhughes934 marked this pull request as ready for review March 30, 2024 21:00
@ldez
Copy link
Member

ldez commented Mar 30, 2024

The validation of the configuration against the JSON Schema is different than using run.
The deprecated options are removed from the JSON Schema (draft-07 doesn't have deprecation support).
The run command (and only the run command) displays deprecation messages, but the validation will display that the properties are missing and will lead to this kind of misunderstanding: #4593

Also, I don't see the point in systematically checking a configuration which most of the time has not changed.
IDEs and text editors allow us to do that live, a call to a pre-commit hook seems overkill.

I'm not a pre-commit user, by choice, maybe there is an interest in this validation hook that I don't see, and I will be happy to learn about it.

@ldez ldez added the feedback required Requires additional feedback label Mar 30, 2024
@matthewhughes934
Copy link
Contributor Author

matthewhughes934 commented Mar 30, 2024

Also, I don't see the point in systematically checking a configuration which most of the time has not changed.

This hook will only be executed via pre-commit run if there's a change in one of the files matching the pattern in files, so it should only check the config if something has changed (though in CI I expect most people to run with pre-commit run --all-files in which case it will always check, but that's just a small bit of extra time in CI which I think is reasonable)

The deprecated options are removed from the JSON Schema (draft-07 doesn't have deprecation support).
The run command (and only the run command) displays deprecation messages, but the validation will display that the properties are missing and will lead to this kind of misunderstanding: #4593

My use case is: I just want to keep my config up-to-date: if a change brings deprecates some option I'm using then I want to be aware of if (and remove it) as soon as I can. This could probably also be solved by just running golangci-lint config verify in CI, but I like the workflow of working with pre-commit

I have no opinion re: potential misunderstandings, if you think this has the potential to be a bother then I have no objections

IDEs and text editors allow us to do that live, a call to a pre-commit hook seems overkill.

This would require everyone working in a project to have their (potentially various) IDEs/editors configured to handle this, vs. just having everyone install pre-commit and have it managed there (unless I misunderstood, in my workflows I tend to prefer tools over trying to support different editors etc. so I'm not familiar with that approach).

@ldez
Copy link
Member

ldez commented Mar 30, 2024

I have no opinion re: potential misunderstandings, if you think this has the potential to be a bother then I have no objections

It's not a blocker, it's more a warning, if there are too many issues on the topic, we will see how to handle that.

This would require everyone working in a project to have their (potentially various) IDEs/editors configured to handle this,

At least for the IDEs I use, there is no configuration required, no specific user action or installation, it's just handled automatically based on the file name.

The configuration file names are defined inside the schema store: https://github.com/SchemaStore/schemastore/blob/c42ad08e78f70a0d37f6fb53a204371ef80bc3d4/src/api/json/catalog.json#L2139-L2143

But I'm not a pre-commit user, and your workflow seems right for a pre-commit user.

@ldez ldez added enhancement New feature or improvement and removed feedback required Requires additional feedback labels Mar 30, 2024
@ldez ldez added this to the next milestone Mar 31, 2024
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez ldez merged commit 7e2229a into golangci:master Mar 31, 2024
13 checks passed
@matthewhughes934 matthewhughes934 deleted the add-config-verify-pre-commit-hook branch March 31, 2024 14:43
@matthewhughes934
Copy link
Contributor Author

It's not a blocker, it's more a warning, if there are too many issues on the topic, we will see how to handle that.

👍 I'd be happy to be pinged/included if any such issues do arise

@ldez ldez modified the milestones: next, v1.58 May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: pre-commit enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants