hadialqattan / pycln

A formatter for finding and removing unused import statements.
https://hadialqattan.github.io/pycln
MIT License
299 stars 18 forks source link

Incompatibility with click-8.1.0 (typer-0.4.0) #116

Closed DiddiLeija closed 2 years ago

DiddiLeija commented 2 years ago

Describe the bug

At https://github.com/wntrblm/nox/pull/590, we tried to adress a Black issue with a breaking Click update. However, we noticed that also pycln was affected.

At least, I can reproduce this with GitHub Actions.

To Reproduce

Steps to reproduce the behavior:

  1. Run pycln anywhere, with the latest click version.
  2. See the error:

    pycln....................................................................Failed
    - hook id: pycln
    - exit code: 1
    
    Traceback (most recent call last):
      File "/home/runner/.cache/pre-commit/repo4hw2hs9j/py_env-python3/bin/pycln", line 5, in <module>
        from pycln.cli import app
      File "/home/runner/.cache/pre-commit/repo4hw2hs9j/py_env-python3/lib/python3.9/site-packages/pycln/__init__.py", line 7, in <module>
        import typer
      File "/home/runner/.cache/pre-commit/repo4hw2hs9j/py_env-python3/lib/python3.9/site-packages/typer/__init__.py", line 12, in <module>
        from click.termui import get_terminal_size as get_terminal_size
    ImportError: cannot import name 'get_terminal_size' from 'click.termui' (/home/runner/.cache/pre-commit/repo4hw2hs9j/py_env-python3/lib/python3.9/site-packages/click/termui.py)
    Traceback (most recent call last):
      File "/home/runner/.cache/pre-commit/repo4hw2hs9j/py_env-python3/bin/pycln", line 5, in <module>
        from pycln.cli import app
      File "/home/runner/.cache/pre-commit/repo4hw2hs9j/py_env-python3/lib/python3.9/site-packages/pycln/__init__.py", line 7, in <module>
        import typer
      File "/home/runner/.cache/pre-commit/repo4hw2hs9j/py_env-python3/lib/python3.9/site-packages/typer/__init__.py", line 12, in <module>
        from click.termui import get_terminal_size as get_terminal_size
    ImportError: cannot import name 'get_terminal_size' from 'click.termui' (/home/runner/.cache/pre-commit/repo4hw2hs9j/py_env-python3/lib/python3.9/site-packages/click/termui.py)

Expected behavior:

  1. Description: I expected pycln to finish cleanly.

  2. Expected output (if present):

  3. Expected fixed code (if present):

Environment (please complete the following informations):

Additional context

I think Click 8.1.0 is the responsible, like the issue with Black (https://github.com/psf/black/issues/2964). However, maybe something else is involved.

henryiii commented 2 years ago

This is https://github.com/tiangolo/typer/issues/377 - as long as they don't bump the version too high when fixing it, it will get fixed here automatically.

This is a great reason to avoid putting upper caps on things - it didn't protect against this, but it might protect against a fix! This would not have been caught (minor release, rather than major, and in a dependency, not this package), so adding upper caps when you knot know there will be a breakage doesn't help. See https://iscinumpy.dev/post/bound-version-constraints/.

DiddiLeija commented 2 years ago

Thanks for explaining this here @henryiii! Indeed, I've just discovered this was an issue with typer.

mecampbellsoup commented 2 years ago

Also experienced this, glad to have found this ticket!

Should we just do e.g. poetry add typer and/or poetry add click to pin the versions used? Or is https://github.com/wntrblm/nox/pull/590/files a better solution?

henryiii commented 2 years ago

If you are using a lock file, it should still have the old versions if you don’t poetry update. All deps are pinned in a lock file.

mecampbellsoup commented 2 years ago
(cloud-app-BL_P3mH3-py3.9) (⎈ minikube:cloud)➜  cloud-app git:(mc/sift-backfill-data) ✗ poetry show click
name         : click
version      : 7.1.2
description  : Composable command line interface toolkit

So I'm assuming I could either specify the version of click as in https://github.com/wntrblm/nox/pull/590/files, or I could lock click in my lockfile to 8.0.4.

henryiii commented 2 years ago

And yes, poetry add “click<8.1” —dev is a short term solution to allow poetry update.

henryiii commented 2 years ago

I’d generally not add development dependencies in a shared environment (like a poetry environment). There no reason you should require click <8.1 because you use an application that requires it. Pre-commit is one way to avoid that. Nox/tox is another.

mecampbellsoup commented 2 years ago

I think I follow what you're saying @henryiii - I ended up just locking the click dependency via the pre-commit config file:

  - repo: https://github.com/hadialqattan/pycln
    rev: v1.1.0 # Possible releases: https://github.com/hadialqattan/pycln/releases
    hooks:
      - id: pycln
        args: [--config=pyproject.toml]
        # TODO: When pycln resolves the issue
        # with click (to be precise, when the typer
        # dep fixes their issue with click), we should remove this
        additional_dependencies: [ "click==8.0.4" ]
        stages: [commit]
henryiii commented 2 years ago

Yep, that's what I'm doing. I have not updated it in the Developer Guidelines yet (https://scikit-hep.org/developer/style#pycln), but I have updated the matching cookie cutter (https://github.com/scikit-hep/cookie/blob/12c4755988652134eeadf82b0b562d02fdcfd7e8/%7B%7Bcookiecutter.project_name%7D%7D/.pre-commit-config.yaml#L63-L69). I'm assuming it will be fixed quickly, but if not, I'll update the dev pages too. :/

henryiii commented 2 years ago

This was fixed in typer 0.4.1, so as long as you aren't getting an old cached version in pre-commit, this should be working again! https://github.com/tiangolo/typer/pull/380

tiangolo commented 2 years ago

I just released Typer 0.4.1 that should handle this. :rocket: :nerd_face:

DiddiLeija commented 2 years ago

Yup. We can close this for now, I think [^1]. Thank you all!

[^1]: Maybe pinning this issue could help people that faced this issue (and avoid extra tickets), but I'll leave it to the maintainers ;)

mecampbellsoup commented 2 years ago

@DiddiLeija how'd you do that cute footnote?! 😅

mecampbellsoup commented 2 years ago

Do y'all set pre-commit autoupdate in your projects out of curiosity?

DiddiLeija commented 2 years ago

@DiddiLeija how'd you do that cute footnote?! 😅

https://github.blog/changelog/2021-09-30-footnotes-now-supported-in-markdown-fields/ :)

mecampbellsoup commented 2 years ago

This was fixed in typer 0.4.1, so as long as you aren't getting an old cached version in pre-commit, this should be working again! tiangolo/typer#380

I cleared my pre-commit repo caches AFAIUI but still hitting the error:

(cloud-app-BL_P3mH3-py3.9) (⎈ minikube:cloud)➜  cloud-app git:(mc/update-precommit-and-run-all-files) ✗ pre-commit gc
9 repo(s) removed.
(cloud-app-BL_P3mH3-py3.9) (⎈ minikube:cloud)➜  cloud-app git:(mc/update-precommit-and-run-all-files) ✗ pre-commit autoupdate
Updating https://github.com/alessandrojcm/commitlint-pre-commit-hook ... already up to date.
Updating https://github.com/pre-commit/pre-commit-hooks ... already up to date.
Updating https://github.com/psf/black ... already up to date.
Updating https://github.com/pycqa/isort ... already up to date.
Updating https://github.com/hadialqattan/pycln ... already up to date.
Updating https://github.com/pre-commit/mirrors-prettier ... already up to date.
(cloud-app-BL_P3mH3-py3.9) (⎈ minikube:cloud)➜  cloud-app git:(mc/update-precommit-and-run-all-files) ✗ pre-commit run --all-files
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check for case conflicts.................................................Passed
black....................................................................Passed
isort....................................................................Passed
pycln....................................................................Failed
- hook id: pycln
- exit code: 1

Traceback (most recent call last):
  File "/Users/mattysoup/.cache/pre-commit/repo1607zjh3/py_env-python3/bin/pycln", line 5, in <module>
    from pycln.cli import app
  File "/Users/mattysoup/.cache/pre-commit/repo1607zjh3/py_env-python3/lib/python3.9/site-packages/pycln/__init__.py", line 7, in <module>
    import typer
  File "/Users/mattysoup/.cache/pre-commit/repo1607zjh3/py_env-python3/lib/python3.9/site-packages/typer/__init__.py", line 12, in <module>
    from click.termui import get_terminal_size as get_terminal_size
ImportError: cannot import name 'get_terminal_size' from 'click.termui' (/Users/mattysoup/.cache/pre-commit/repo1607zjh3/py_env-python3/lib/python3.9/site-packages/click/termui.py)

I just removed the pinned click version from the .pre-commit-config.yaml section for pycln:

commit ab702250fe096bbda57484f4c9f43ddd1a2135f6 (HEAD -> mc/update-precommit-and-run-all-files)
Author: Matt Campbell <mecampbell25@gmail.com>
Date:   Thu Mar 31 09:37:04 2022 -0400

    refactor(project): Remove pinned typer dep in pycln pre-commit

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index c3ca2cb..b187b25 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -31,10 +31,6 @@ repos:
     hooks:
       - id: pycln
         args: [ --config=pyproject.toml ]
-        # TODO: When pycln resolves the issue
-        # with click (to be precise, when the typer
-        # dep fixes their issue with click), we should remove this
-        additional_dependencies: [ "click<8.1" ]
         stages: [ commit ]
   - repo: https://github.com/pre-commit/mirrors-prettier
     rev: v2.6.1 # Use the sha or tag you want to point at
henryiii commented 2 years ago

pre-commit clean?

Also, pre-commit autoupdate is not a setting, it just updates all the repos when you run it.

henryiii commented 2 years ago

Most of my repos use pre-commit.ci, which runs pre-commit autoupdate once a week (can be set to monthly) and PRs the result if it changes.

mecampbellsoup commented 2 years ago

pre-commit clean?

Also, pre-commit autoupdate is not a setting, it just updates all the repos when you run it.

You are a saint and a scholar!