sk- / git-lint

improving source code one step at a time
Apache License 2.0
236 stars 78 forks source link

Add --config parameter for custom config path #116

Closed mattolenik closed 6 years ago

mattolenik commented 6 years ago

New --config= parameter that allows overriding the location of .gitlint.yaml and specify any file.

sk- commented 6 years ago

Hi Matthew, thanks for the PR, could you explain a little bit more why a config option is needed and why having the config checked in at the root of the repo is not enough.

mattolenik commented 6 years ago

I found it necessary because our build and linting workflow requires it. We have enormous code duplication of .gitlint.yaml across our repos which I am trying to reduce. My goal is to have git-lint run in a container, which can provide a pre-baked default .gitlint.yaml, and then merge it with any config in the repo. But since git-lint only searches these two locations, and because I have to mount the whole repo into the countainer for git-lint to work, it means that the merging of the config persists back to the host, out of the container, leaving the repo's config in a dirty state. My solution was to just store the config somewere else and load it with a parameter.

Passing custom config location is a pretty common pattern across many tools. In complex processes the default assumptions of many tools often aren't enough.

sk- commented 6 years ago

Why do you need to merge the configurations?

Have you considered having your duplicated configuration files in a git submodule, linked symbolically? If so, why wasn't that a satisfactory solution.

The problem of having a configuration file outside of the repo is that developers will need to always remember to type git lint --config=<path-to-config>, which in turn would make some of the tools, like precommit check, to misbehave.

So, if one of your developers runs git lint they will get the default configuration, which may be quite different to what you have in the external config file. In which case, they would spend lot of extra time, fixing something that wasn't even required in the first place.

Maybe we coudl

mattolenik commented 6 years ago

The merge is to enable repo owners to specify additional or override configs. We have some base standard we want across the board but individual repos also need specific things. Git submodules won't solve that problem.

How would the extra parameter make tools misbehave? For my application (and many others) the tool is being called by another tool, nothing for developers to forget.

This doesn't seem to be a problem with countless other tools that have config override. We can't insulate developers from making all kinds of mistakes. At some point they do indeed need to try

sk- commented 6 years ago

The problem is that developers working on your code base need to remember to specify --config <path_to_config> when running git lint, and the pre commit hook would need to be updated to reflect that configuration is being used as well.

What about in your CI setup you create a symbolic link to the merged config.

Something like

merge_gitlint_config base.yml repo.yml > /tmp/merged.yml
ln -s /tmp/merged.yml /path/to/repo/.gitlint.yml
git lint
rm /path/to/repo/.gitlint.yml
mattolenik commented 6 years ago

I could do that, but I didn't want to, I wanted to add the feature to the tool. git-lint, nor really any linting tool, should be run by hand, it should be run as part of a build. Developers could just as easily forget to run git-lint entirely. This is why hooks and gated CI exists. Why is updating the precommit hook a problem? Precommit hooks are updated as tools they use are updated...and? Isn't that obvious? So, the feature should be rejected because someone may, in the course of starting to use it, forget to update part of their code? I'm not sure I understand the resistance to the change, this does not seem like any substantial failure case. But if you disagree with the change I'm fine using a workaround, too.