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

Latest release of Black does not respect [--config .pyproject.toml] setting in pre-commit #1667

Closed
potiuk opened this issue Sep 2, 2020 · 16 comments
Labels
T: bug Something isn't working

Comments

@potiuk
Copy link

potiuk commented Sep 2, 2020

Describe the bug A clear and concise description of what the bug is.

To Reproduce Steps to reproduce the behavior:

  1. Checkout latest "Apache/Airflow" sources https://github.com/apache/airflow/
  2. Run pre-commit install
  3. Run pre-commit run black --all-files
  4. Check succeeds
  5. Change black revision in .pre-commit-config.yaml to 'stable' or '20.8b1' or 'master' (currently it is '19.10b0')
  6. Run pre-commit install
  7. Run pre-commit run black --all-files
  8. You will see a lot of files updated due to changed line length

We keep the line length in pyproject.toml and we even force the pyproject.toml file via extra arg:

  - repo: https://github.com/psf/black
    rev: 19.10b0
    hooks:
      - id: black
        files: api_connexion/.*\.py|.*providers.*\.py
        exclude: .*kubernetes_pod\.py|.*google/common/hooks/base_google\.py$
        args: [--config=./pyproject.toml]

Seems that the behaviour changed very recently - it was working before and people who installed pre-commit recently started to complain about it:

Expected behavior

The line-lenght in pyproject.toml should be respected in black's pre-commit.

Environment (please complete the following information):

  • Version: master, 20.8b1, stable, 20.8b0.
  • MacOS and Python version: Python 3.6.10

Does this bug also happen on master?

Yes. It behaves the same with master set as revision.

Additional context Add any other context about the problem here.

Our pyproject.toml file:

[tool.black]
line-length = 110
target-version = ['py36', 'py37', 'py38']
skip-string-normalization = true

Also tried it with:

    hooks:
      - id: black
        files: api_connexion/.*\.py|.*providers.*\.py
        exclude: .*kubernetes_pod\.py|.*google/common/hooks/base_google\.py$
        args:
          - "--config"
          - "./pyproject.toml"
@potiuk potiuk added the T: bug Something isn't working label Sep 2, 2020
@potiuk
Copy link
Author

potiuk commented Sep 2, 2020

Convirmed that it also does not work with 20.8b0

@potiuk potiuk changed the title Latest releases of Black do not respect [--config .pyproject.toml] setting in pre-commit Latest releases of Black does not respect [--config .pyproject.toml] setting in pre-commit Sep 2, 2020
@potiuk
Copy link
Author

potiuk commented Sep 2, 2020

I suspect that internally pre-commit started using blackd but this is a wild guess:

Please note blackd will not use pyproject.toml configuration.

@mdkess
Copy link

mdkess commented Sep 2, 2020

I run into the same issue just passing --line-length=120 in pre-commit config.

@potiuk potiuk changed the title Latest releases of Black does not respect [--config .pyproject.toml] setting in pre-commit Latest release of Black does not respect [--config .pyproject.toml] setting in pre-commit Sep 2, 2020
@asottile
Copy link
Contributor

asottile commented Sep 8, 2020

nothing has changed in the pre-commit configuration, I suspect you're being confused by the new trailing commas behaviour and not the args

here's a demo showing that args are passed properly for --line-length

$ cat .pre-commit-config.yaml 
repos:
-   repo: https://github.com/psf/black
    rev: 20.8b0
    hooks:
    -   id: black
        args: [--line-length=120]
$ wc -c t.py
94 t.py
$ cat t.py 
print("long", "long", "long", "long", "long", "long", "long", "long", "long", "long", "long")
$ pre-commit run --all-files
black....................................................................Passed
$ sed -i '/args/d' .pre-commit-config.yaml 
$ cat .pre-commit-config.yaml
repos:
-   repo: https://github.com/psf/black
    rev: 20.8b0
    hooks:
    -   id: black
$ pre-commit run --all-files
black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted t.py
All done! ✨ 🍰 ✨
1 file reformatted.

$ cat t.py 
print(
    "long",
    "long",
    "long",
    "long",
    "long",
    "long",
    "long",
    "long",
    "long",
    "long",
    "long",
)

here's a demo showing that pyproject.toml works

(note that --config=pyproject.toml is unnecessary, black will pick up the file automatically)

$ cat .pre-commit-config.yaml 
repos:
-   repo: https://github.com/psf/black
    rev: 20.8b0
    hooks:
    -   id: black
$ cat pyproject.toml 
[tool.black]
line-length = 110
$ cat t.py 
print("long", "long", "long", "long", "long", "long", "long", "long", "long", "long", "long")
$ wc -c t.py
94 t.py
$ pre-commit  run --all-files
black....................................................................Passed
$ sed -i '/line-length/d' pyproject.toml 
$ cat pyproject.toml
[tool.black]
$ pre-commit  run --all-files
black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted t.py
All done! ✨ 🍰 ✨
1 file reformatted.

$ cat t.py 
print(
    "long",
    "long",
    "long",
    "long",
    "long",
    "long",
    "long",
    "long",
    "long",
    "long",
    "long",
)

@potiuk
Copy link
Author

potiuk commented Sep 8, 2020

Indeed that could explain it - seems like all the cases are about training commas.

It was a bit unexpected though. I think others might have similar cases - we have +4867/-1357 change in Apache Airflow with this. I just wondered if there is a way to warn the users somehow ?

@potiuk potiuk closed this as completed Sep 8, 2020
@asottile
Copy link
Contributor

asottile commented Sep 8, 2020

@kaxil
Copy link

kaxil commented Sep 8, 2020

nothing has changed in the pre-commit configuration, I suspect you're being confused by the new trailing commas behaviour and not the args

Is there a flag to control those trailing commas?

@asottile
Copy link
Contributor

asottile commented Sep 8, 2020

Is there a flag to control those trailing commas?

😆 kind of the point of black is there's no options

@kaxil
Copy link

kaxil commented Sep 8, 2020

😄 thought so that might be the answer - 'uncompromising'

Thanks, we love 'black' and we will soon enable it across entire Apache Airflow (https://github.com/apache/airflow/commits/) code base

@potiuk
Copy link
Author

potiuk commented Sep 9, 2020

Agree. We do love black :). I just have not realised it is a "beta" product ;). But it took some 12 years for Gmail to get out Beta, so I think we can wait.

@asottile Honestly - as @kaxil said - we love black. My point about the "informing user" is that we both with @kaxil got confused where it came from and until we got the answer from you, we were 100% sure that it's because of the line length. We even tried hard to enable the right line length and failed. So my point is - in this case when this change is so significant, maybe on top of mentioning it as one of the changes in the changelog (without clear information that it has that much of an impact). there could be a way to kind of warn the user what's going on ? Just explain the reason why suddenly something changed.

I guess it could be possible to detect that the change applied to 100 files is mostly because of the change in new version and some message at the end "The changes above were introduced by the new rule ["link to description here"] ".

Just a thought. That could save you from having to get/answer the issue and possibly in the next few weeks you will have to do it few more times for others - so investing in this kind of "information" for the user might pay-off rather quickly.

@potiuk
Copy link
Author

potiuk commented Sep 9, 2020

And really - good intentions here :). We are both with @kaxil long-time python develiopers and commiters and PMC in Apache Airflow project, and we have a looooooot of automation for our code (we have more than 50 pre-commit checks in Apache Airflow https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#pre-commit-hooks but this one was confusing as hell)

@asottile
Copy link
Contributor

asottile commented Sep 9, 2020

yeah to be honest, I'm not affiliated with black in any way, I just periodically go through and search for the pre-commit-related issues here and try and help out (since I'm the pre-commit creator)

@potiuk
Copy link
Author

potiuk commented Sep 9, 2020

Aaaah so we meet again :). I have an outstanding feature or two to commit to pre-commit @asottile - i realised I know you from somewhere. BTW. The "fail-fast" is not that important any more but one that might be useful (especially if you have 50+ pre-commits) is to get auto-complete for the checks and a way to export the list of pre-commits in a parseable way.

We somehow handle it.... I developed a "Breeze" development environment ("It's a Breeze to develop Airflow") and we have auto-complete for all pre-commit checks and ..... wait for it ...... ... wait for it ........ we are using pre-commit check to see if all the pre-commits are properly documented and are added to the auto-complete list :) . I just merged this commit yesterday apache/airflow#10789

@potiuk
Copy link
Author

potiuk commented Sep 9, 2020

And BTW. once again - thanks for pre-commit. It's a game-changer in automation of software development.

@kaxil
Copy link

kaxil commented Sep 9, 2020

Agree with Jarek, @asottile Thanks for pre-commit 🙏

@asottile
Copy link
Contributor

asottile commented Sep 9, 2020

and we have auto-complete for all pre-commit checks and ..... wait for it ...... ... wait for it ........ we are using pre-commit check to see if all the pre-commits are properly documented

that's funny, I've done the same thing here: https://github.com/pre-commit/pygrep-hooks/blob/eae6397e4c259ed3d057511f6dd5330b92867e62/.pre-commit-config.yaml#L38-L46

glad you like the tool <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants