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

Move tool configuration from .pre-commit-config.yaml to pyproject.toml #50

Closed
plule-ansys opened this issue Apr 6, 2022 · 7 comments
Closed
Labels
enhancement New features or code improvements

Comments

@plule-ansys
Copy link
Contributor

plule-ansys commented Apr 6, 2022

When configuring an IDE, it's common to use "format on save" feature that triggers linter each time you save. Unfortunately, in my case, black would format to its default of 88 character per lines, that were clashing with the pre-commit's 100 characters.

If this configuration is moved into pyproject.toml, then both pre-commit and the IDE (or just black called independently) are able to interpret it, solving the issue.

FYI, I've applied this fix in ansys/pypim#6 This fix omits pydocstyle as I'm not very confortable with this tools usage.

@jorgepiloto
Copy link
Member

Thanks for opening this, @plule-ansys. I understand the main problem you expose here...

It looks like it is not possible to define pre-commit configuration in pyproject.toml files, see this comment pre-commit/pre-commit#1165 (comment). However, from ansys/pypim#6 (and after checking locally too) it is clear that it is possible to combine both pre-commit and pyproject.toml.

From my point of view, all code style configuration must be collected under a common file, the pre-commit-config.yaml one. This introduces safety: all conf is declared in a single file.

In the end, this is not an issue but something related with a particular IDE developers may be using. We must be as independent from IDEs as possible.

@greschd
Copy link
Member

greschd commented Apr 6, 2022

From my point of view, all code style configuration must be collected under a common file, the pre-commit-config.yaml one. This introduces safety: all conf is declared in a single file.

I'd have to disagree quite strongly with that:

In the end, this is not an issue but something related with a particular IDE developers may be using. We must be as independent from IDEs as possible.

The pyproject.toml format is as close to a commonly accepted standard as we're going to get. On the other hand, .pre-commit-config.yaml is somewhat niche.

The main difference however is not one config file vs. another: In .pre-commit-config.yaml, we're hiding the config values away as command line arguments. Third-party tools, or even a plain invocation of black from the command line, have no way of finding that.

To contrast, if we put the configuration of pyproject.toml, any invocation of the tool will pick it up. So my preference would be: As much as possible into pyproject.toml (or, if the tool doesn't support it, a tool-specific config file). Only what really controls the pre-commit step should go into .pre-commit-config.yaml.

@greschd greschd reopened this Apr 6, 2022
@jorgepiloto
Copy link
Member

We're hiding the config values away as command line arguments. Third-party tools, or even a plain invocation of black from the command line, have no way of finding that.

This is a strong point, @greschd. I rushed into the decision of closing this issue... Sorry for that, @plule-ansys.

It is true that pyproject.toml is an official file. Lots of the tools out there already support specifying their config via this file. An important PEP about this is PEP 680:

Alright! Key points to implement, as @greschd said:

  • Tools config must be specified in pyproject.toml (when possible).
  • Only what really controls the pre-commit step should go into .pre-commit-config.yaml.

@plule-ansys plule-ansys added the enhancement New features or code improvements label Apr 7, 2022
@jorgepiloto
Copy link
Member

Before implementing this, I will address #48 first, so everything is unified under a common pyproject.toml.

@jorgepiloto
Copy link
Member

This was partially addressed in #53. Labeling it now as "blocked" till PyCQA/flake8#234 and codespell-project/codespell#2055 are implemented.

@plule-ansys
Copy link
Contributor Author

@jorgepiloto As far as I'm concerned, I think the issue can be closed. The critical path is for tools that modify your code. The tools that just run checks are less likely to be used externally.

@jorgepiloto
Copy link
Member

Thanks for the comment, @plule-ansys. I though it would be useful to have an issue remembering that not all tools configuration were moved to the pyprojec.toml file.

Anyway, I agree with your point. We can close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features or code improvements
Projects
None yet
Development

No branches or pull requests

3 participants