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

support new config file format #319

Closed
fantua opened this issue Aug 12, 2022 · 5 comments · Fixed by #520
Closed

support new config file format #319

fantua opened this issue Aug 12, 2022 · 5 comments · Fixed by #520

Comments

@fantua
Copy link
Contributor

fantua commented Aug 12, 2022

It will be really cool to add .lefthook.yml (with dot in the beginning) config file format support. Just to align with other libs:

@mrexox
Copy link
Member

mrexox commented Aug 13, 2022

Sounds cool! But I have concerns about how it would play with overriding feature. I mean, we support lefthook.yml as global config and lefthook-local.yml as local config which overrides values from global config.

It would be confusing if we have both .lefthook.yml and lefthook.yml. Main questions is : how should we merge them? What config would be the "main"? Wouldn't it be confusing?

@fantua
Copy link
Contributor Author

fantua commented Aug 16, 2022

Good point! I think for this case we shouldn’t support overriding feature. We just need to define priority order and use only one config file. Same as eslint currently does:

If there are multiple configuration files in the same directory, ESLint will only use one. The priority order is as follows:

For global it could be something like this:

  1. lefthook.yml (potentially this format can be deprecated and removed in v2)
  2. .lefthook.yml
  3. .lefthook.json (maybe it's a good idea to add json format support)

For local:

  1. lefthook-local.yml
  2. .lefthook.local.yml (see CRA for .local reference)
  3. .lefthook.local.json

@haysclark
Copy link

haysclark commented Dec 23, 2022

It seems like it would be best to treat any config file without -local as a global config.

You could get the core config file path via this regex [regex101]:

^\.?lefthook\.(yml|yaml|json)

And any local configs via this regex [regex101]:

^\.?lefthook-local\.(yml|yaml|json)

Valid example

All of these scenarios would be supported:

current

.
├── lefthook-local.yaml
└── lefthook.yaml

new dot configs

.
├── .lefthook-local.yaml
└── .lefthook.yaml

mixed

.
├── lefthook-local.yaml
└── .lefthook.yaml

For config files conventions are pretty common these days.


Invalid example

Multiple global or local configs would throw an error.

.
├── lefthook.yaml
└── .lefthook.yaml

# Error: invalid configuration, too many files.

@mrexox
Copy link
Member

mrexox commented Dec 23, 2022

Wow, it makes sense! What do you think about default config file? I guess we could change it to be .lefthook.yml in v2 but still support lefthook.yml for existing projects.

Why do you think we should support JSON format? What is the potential use case?

Do you have a will to prepare a draft PR for this change?

@technicalpickles
Copy link
Contributor

Good point! I think for this case we shouldn’t support overriding feature. We just need to define priority order and use only one config file.

Something I would add to this is to display a warning when multiple global config files are found, and say which is being used. This could be a pretty frustrating situation when you have both, trying to iterate on the file, and not see your changes taking an effect until you eventually discover that a different file is loading.

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 a pull request may close this issue.

4 participants