hakancelikdev / unimport

:rocket: The ultimate linter and formatter for removing unused import statements in your code.
https://unimport.hakancelik.dev/
MIT License
239 stars 22 forks source link

`unimport` crashes on py3.12 due to `distutils` removal #297

Closed mxr closed 12 months ago

mxr commented 1 year ago

Repro steps:

Environment (from here):

$ cat example.py
import sys
print('hello world')
$ pip freeze | rg unimport
unimport==1.0.0
$ unimport example.py
Traceback (most recent call last):
  File "/Users/mxr/tmp/venv3.12/bin/unimport", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/Users/mxr/tmp/venv3.12/lib/python3.12/site-packages/unimport/__main__.py", line 4, in main
    from unimport.main import Main
  File "/Users/mxr/tmp/venv3.12/lib/python3.12/site-packages/unimport/main.py", line 6, in <module>
    from unimport import commands, utils
  File "/Users/mxr/tmp/venv3.12/lib/python3.12/site-packages/unimport/commands/__init__.py", line 1, in <module>
    from unimport.commands import options
  File "/Users/mxr/tmp/venv3.12/lib/python3.12/site-packages/unimport/commands/options.py", line 5, in <module>
    from unimport.config import Config
  File "/Users/mxr/tmp/venv3.12/lib/python3.12/site-packages/unimport/config.py", line 14, in <module>
    from unimport import constants as C
  File "/Users/mxr/tmp/venv3.12/lib/python3.12/site-packages/unimport/constants.py", line 2, in <module>
    import distutils.sysconfig
ModuleNotFoundError: No module named 'distutils'

See https://docs.python.org/3/whatsnew/3.10.html#distutils-deprecated for more info

hakancelikdev commented 1 year ago

Hi thanks for the issue but

officially, unimport does not yet support versions after Python 3.10. Wouldn't this fix be a bit pointless when no development has been made yet? It would be better to develop support for Python3.11 and Python3.12 first.

mxr commented 1 year ago

Do you mind clarifying? I believe unimport already works on py3.11 (at least tox -e py3.11 passed and the wheel is not limited to <py3.10), so supporting it would simply require changing CI parameters. And if desired, supporting py3.12 can happen with the 2 PR's I have open.

mxr commented 1 year ago

Phrased another way - if we don't want to support py312 then the right thing is to configure that in python_requires (currently we allow an install but crash at runtime)

hakancelikdev commented 1 year ago

If all packages support Python 3.11 and 3.12, we can make the necessary changes in Unimport and provide support for these versions, there is no problem with this.

Do you want to do this job or should I do it?

mxr commented 1 year ago

I don't know what you mean by "if all packages". According to https://endoflife.date/python python3.11 and python3.12 are supported officially by Python maintainers

I can update this project to run in CI for python 3.11

hakancelikdev commented 1 year ago

When I say all packages, I mean other 3rd party packages that unimport uses.

hakancelikdev commented 1 year ago

You don't need to do anything additional for now, I will check your PR.

mxr commented 1 year ago

I think we are mixing up two things

A) Whether unimport runs under python3.11+? B) Whether unimport can process syntax available in python3.11+?

(A) is enforced by setting python_requires. Since unimport sets python_requires = >=3.6, users expect that the tool runs under all of those versions. One way to resolve this bug report is to remove usage of distutils in favor of other libraries. Another way is to set a ceiling on python_requires and keep using distutils

(B) does not have a standard way of enforcement. At the time of the bug report in #291, the match statement (new to py310) could not be processed by libcst even though unimport does run on py310. One way to solve this class of issue is to add a new target_version argument to unimport in order to be explicit/implicit about which syntax you support

I opened this ticket in response to issue (A) but it sounds like you want to resolve both (A) and (B)? What I can do is check if the dependencies run on py311 and/or set a ceiling for python_requires if you feel this better guards against surprises

hakancelikdev commented 1 year ago

Yes, I understand you, you are right.

1 -) There was a mistake when determining python_requires, we should update it to include only versions that we guarantee to support.

2 -) If we want to support Python 3.11 or a higher Python version, we must check the packages we currently use, update our setup file, add new tests to ensure that we support new features, and fix any issues related to these versions, if exists.

There are two separate jobs, they must be done in order.

mxr commented 1 year ago

I think we are still not totally aligned, or at least the use of 'supports' is overloaded. I may be guilty of this too.

Let's use the terminology "runs under" and "language version" to delineate the two. For example

Now applied to unimport: unimport is configured to run under python 3.8. However it fails to run under 3.12 or higher - that's what this issue is about. Separately, it crashes on language version 3.10 or higher (#291).

It would be straightforward to fix unimport so it runs under python3.12 by removing distutils. I have a PR for that. It would take more work to support language versions 3.10+ and not something I can take on.

Let me know your thoughts

hakancelikdev commented 12 months ago

Ok I understand, thank you for your effort, and sorry for getting back late.

hakancelikdev commented 12 months ago

Now you can install and use version 1.0.1 -> https://pypi.org/project/unimport/1.0.1/

mxr commented 12 months ago

Thanks!