Skip to content

feat(config): look up from .config subdir by default, too #933

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

scop
Copy link
Contributor

@scop scop commented Jan 19, 2025

Closes # (issue)

⚡ Summary

Look up configs from .config subdirs too, to help unclutter project top level dirs, https://github.com/pi0/config-dir (and other similar proposals)

pi0/config-dir support discussion item: pi0/config-dir#6 (comment)

This is a draft for discussion purposes, to be still done at least

  • tests
  • the .config/lefthook and .config/lefthook-local source dirs should work out of the box, too
  • decide how relative paths should work and make sure they do that way, e.g. should they always be from the "top level dir" no matter if the config is in .config, or relative to that dir (I think I'd propose the former)
  • remove with uninstall

☑️ Checklist

  • Check locally
  • Add tests
  • Add documentation

@scop scop marked this pull request as draft January 19, 2025 09:35
@mrexox
Copy link
Member

mrexox commented Jan 20, 2025

Hey! This sounds good! Could you also add the paths to uninstall?

I don't think moving scripts to .config/ is a good idea, since scripts are not configs actually, they must belong to repo root.

Regarding the paths I think it's better consider lefthook as being run from git project root always, for simplicity. So moving a file from repo root to .config/ doesn't make you change the config adding ../ to the root option, etc.

@mrexox
Copy link
Member

mrexox commented Feb 28, 2025

Hey @scop! Do you want to continue on this PR? If not, can I apply the fixes myself and merge it?

@ThomasSanson
Copy link

ThomasSanson commented Apr 22, 2025

Hello @mrexox,

I'm following up on this pull request. It would be excellent for us if the configuration could also be placed in a subdirectory of .config, such as .config/lefthook/lefthook.yml. More generally, I am thinking of something like .config/**/lefthook.yml.

Additionally, would a --config option for lefthook, allowing the explicit specification of the configuration file's location, be a more generic and straightforward solution ?

If there's a need for contributions to the pull request, I’d be happy to volunteer.

@mrexox
Copy link
Member

mrexox commented Apr 22, 2025

Hey @ThomasSanson! This PR adds support for .config/lefthook{.yml,.toml,.json}, not exactly what you wanted. Does it work for you? If it does I can make an effort on finishing this PR

@scop
Copy link
Contributor Author

scop commented Apr 26, 2025

Thanks for considering. I can try to find time sometime to work on this, but I don't know when that might be, so it's very much appreciated if you @mrexox can chime in with the implementation here.

Just one remark to @ThomasSanson's message

Additionally, would a --config option for lefthook, allowing the explicit specification of the configuration file's location, be a more generic and straightforward solution ?

No objections here if someone wants to add that, but the whole point of my request is to have .config be a location that is looked up by default, without having to specify any configuration or command line options. See the paragraph starting with "Some tools offer command line ..." at https://github.com/pi0/config-dir#motivation for the rationale.

@ThomasSanson
Copy link

Hello @scop,

Apologies for the additional comments I made on this pull request. I intended to benefit from the ongoing work without being too intrusive. I'll wait for this PR to be merged and then submit a separate one for my specific needs. 😇

@scop
Copy link
Contributor Author

scop commented Apr 26, 2025

No problem, I just wanted to clarify my own intent with this, as I did not make it that clear in writing here but just pointed to external resources.

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.

3 participants