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

Configure the language(s) in the pre-commit hook #447

Closed
yannham opened this issue Apr 27, 2023 · 9 comments
Closed

Configure the language(s) in the pre-commit hook #447

yannham opened this issue Apr 27, 2023 · 9 comments
Assignees

Comments

@yannham
Copy link
Member

yannham commented Apr 27, 2023

Is your feature request related to a problem? Please describe.
Topiary is expected to be multi language, and the number of supported languages will surely grow. In that situation, it can happen that you want to use Topiary e.g. only for one or a restricted number of languages but use other formatters for other languages. Typically, topiary is the only formatter able to handle Nickel right now, but I don't want it to try to format the bash scripts of the Nickel repo.

Describe the solution you'd like
Make the exported pre-commit-hook a function that take as an input the language to enable, or to exclude. In the long term, it could be even nicer to have a module-like approach (which I believe is the case for built-in pre-commit-hooks), but as long as it's parametrizable, it would already be awesome.

Additional context
I can take a stab at implementing this change, but I won't have the bandwith in the coming weeks, I fear.

@Niols Niols self-assigned this May 2, 2023
@Niols
Copy link
Member

Niols commented May 2, 2023

pre-commit-hooks already allow you to customise its hooks, so I believe what you're asking for is already doable. For instance, with the official hooks, instead of

topiary.enable = true;

you can do

topiary = {
  enable = true;
  files = "(\\.sh$)|(\\.mli$)";
};

which overrides the default pre-commit's behaviour so that it only runs on .sh and .mli files (why you'd want that - nobody knows). If you are using the pre-commit hook from our flake, this would be

topiary-latest = topiary.lib.${system}.pre-commit-hook // { files = ... };

What do you think of this solution? I can see how that maybe does not fully solve your problem, so I would be interested in hearing what shortcomings you see to this!

@yannham
Copy link
Member Author

yannham commented Jun 21, 2023

I tried and it fits the bill for my simple use-case, thanks. For the record, if you want to use the latest topiary from Topiary's flake, you should rename the entry inside pre-commit-hooks (I used topiary-latest, but whatever works), otherwise Nix complains about one option having conflicting declarations, because topiary is already defined by pre-commit-hooks.

@Niols I let you decide if this issue should stay open or not, if you think a better solution is mandated, but as far as I'm concerned, my problem is solved.

@Niols
Copy link
Member

Niols commented Jul 5, 2023

I tried and it fits the bill for my simple use-case, thanks. For the record, if you want to use the latest topiary from Topiary's flake, you should rename the entry inside pre-commit-hooks (I used topiary-latest, but whatever works), otherwise Nix complains about one option having conflicting declarations, because topiary is already defined by pre-commit-hooks.

I'm glad this solves your issue! I edited my message above to use topiary-latest. As you noticed, there is a name clash otherwise.

@Niols I let you decide if this issue should stay open or not, if you think a better solution is mandated, but as far as I'm concerned, my problem is solved.

I think we are touching a bigger discussion about behaviour of the CLI, configuration, etc. I wouldn't want to close this issue without having moved the insights gained from it to another discussion. @ErinvanderVeen Is there such an issue/discussion somewhere?

@ErinvanderVeen
Copy link
Collaborator

I've opened a discussion on #561.

I would like to say that Topiary currently also reads the languages.toml file present in .topiary/languages.toml in the current directory or any above it. Here you could disable automatic language detection by setting the extensions field to the empty list:

[[language]]
name = "bash"
extensions = []

@Niols
Copy link
Member

Niols commented Jul 6, 2023

I would like to say that Topiary currently also reads the languages.toml file present in .topiary/languages.toml in the current directory or any above it.

I think this is the right answer, actually, and this is what you should do, @yannham. That way, you can ensure that Topiary as run by contributors and Topiary as run by pre-commit hooks use the exact same configuration. If there was a way to define which languages to support positively (instead of disabling extensions of languages), then that would entirely solve your issue in a clean way, I think.

@yannham
Copy link
Member Author

yannham commented Jul 7, 2023

So, IIUC, I should revert my change and just use the plain hook from pre-commit-hook, and add a file .topiary/languages.toml. But I'm not sure to understand the latter format: does the example given by @ErinvanderVeen enable only bash, or disable only bash? If it's a blacklist process, it's not very maintainable, as I need to keep up to date with supported languages and disable them all but Nickel.

@Niols
Copy link
Member

Niols commented Jul 7, 2023

So, IIUC, I should revert my change and just use the plain hook from pre-commit-hook, and add a file .topiary/languages.toml.

I think that would be the best option.

But I'm not sure to understand the latter format: does the example given by @ErinvanderVeen enable only bash, or disable only bash? If it's a blacklist process, it's not very maintainable, as I need to keep up to date with supported languages and disable them all but Nickel.

It is also something that I wonder now that I am looking at how configuration works. I feel configuration files do not replace the default configuration but only add to it (potentially nullifying some values in it), and I don't like that so much either.

@ErinvanderVeen
Copy link
Collaborator

ErinvanderVeen commented Jul 7, 2023

Hej! Definitely not, and we need to implement a better way to override the configuration file. This was only meant as a possible workaround, and definitely not meant to be sustainable.

I'm working on #564 right now, whatever comes out of that should be the right approach for you.

So for now, use the commit-hook configuration.

@ErinvanderVeen
Copy link
Collaborator

(draft) PR is open here: #567

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

No branches or pull requests

3 participants