search

MichaelKim0407 / flake8-use-fstring

MIT License
51 stars 7 forks source link

Pyproject wheel #32

Closed jmahlik closed 11 months ago

jmahlik commented 1 year ago

Changes to the build to ship a wheel. Let me know if there's any modifications you'd like to see, happy to make them.

Closes #31

MichaelKim0407 commented 1 year ago

Thanks for your PR. A few thoughts.

  1. I have previously indicated in another PR that I don't want to move from setup.py to build because I'm not familiar with it. As the maintainer I feel the responsibility that I should understand what is going on with it. But unfortunately I haven't had much time learning the new ecosystem. Thank you for the linked article - I took a brief look, and I'll try to find time to read it. This plugin is pretty much maintenance only, not having seen any significant changes over the years, so I'd err on the side of "if it ain't broke, don't fix it". I understand from your #31 that you originally wanted to have a wheel, and I believe setup.py can build a wheel. So is it ok to just add a wheel to the build with setup.py?
  2. Related to this. Why does import __version__ not work with build? If you want to move to build, is there a way to make import __version__ work? It's obviously a lot cleaner than processing the text of a .py file.
  3. On the removal of flake8-commas. Again, this plugin is maintenance only. flake8-commas is used by this project for quality control and not installed for end users, so I am against removing it, especially since it still works just fine. If you would like to add 3.12 tests, I would suggest creating a separate ci pipeline for 3.12 instead of removing the check from all Python versions.

I know I might sound very reluctant to changes, but it's because I'm the maintainer and my priority is that my library works with minimal attention.

jmahlik commented 1 year ago

Good points. Sorry, I didn't see the prior PR. Totally understandable. I'll make some adjustments.

For point 2: build does the build in a separate environment, similar to pip. The package isn't installed in that isolated environment so it can't be imported. I mostly implemented the version parsing so that things don't break if pip stops invoking the setup.py directly in the future.

jmahlik commented 1 year ago

Made some changes. Let me know thoughts, happy to adjust.

MichaelKim0407 commented 12 months ago

Thanks again. I'm happy to remove 3.6. Also flake8-commas < 3.12 is a good solution, thank you for finding it.

I'll try to take a look at the version import myself in the next few days and see what I can find.

MichaelKim0407 commented 12 months ago

I did some digging

  1. from flake8_use_fstring import __version__ works fine with python setup.py sdist bdist_wheel so if we are sticking with that, I don't really want to use get_version text processing, as (again as I said above) using an import is a lot cleaner.
  2. I tried using build, and interestingly from flake8_use_fstring import __version__ also works if you remove the pyproject.toml file you introduced. I haven't found out why pyproject.toml breaks it.

My suggestion for this PR: I would like to merge the changes that you made, however (I'm somewhat stubborn on this) I really don't like it when one PR contains different unrelated issues. For now I am happy to accept these two changes

For the diffs that are irrelevant to these two changes (pyproject.toml and get_version) please remove them so I can merge this PR. If you are interested in discussing about using build, pyprojec.toml, or anything related, please move to #33 .

Thank you!

jmahlik commented 11 months ago

I updated with the wheel and python 3.12 tests. We can move the discussion on pyproject.toml/build over to #33.