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

Explore using isort to manage imports #31

Merged
merged 7 commits into from Feb 4, 2023
Merged

Explore using isort to manage imports #31

merged 7 commits into from Feb 4, 2023

Conversation

ross
Copy link
Contributor

@ross ross commented Jan 20, 2023

Fairly happy with the results here. If we want to go forward with this it'll need ported over into https://github.com/octodns/octodns-template and https://github.com/octodns/octodns and then applied to a whole bunch of repos.

/cc #29 (comment) @bkane-msft for the suggestion

@ross ross self-assigned this Jan 20, 2023
@bkane-msft
Copy link
Contributor

bkane-msft commented Jan 20, 2023

Oh I love that you're consolidating things in pyproject.toml! I'm using VS Code with (among others) the black and isort plugins and now I won't have to disable those to use octodns-specific settings (the plugins will read from pyproject.toml settings).

You might also consider adding the following and simplifying pytest:

[tool.pytest.ini_options]
pythonpath = "."

Then you can remove the export PYTHONPATH=.:$PYTHONPATH line in ./script/test and script/coverage

Docs link: https://docs.pytest.org/en/6.2.x/customize.html#pyproject-toml

@bkane-msft
Copy link
Contributor

In fact, for recent (maybe too recent for octo's taste) versions of setuptools and Python, it's possible to use ONLY pyproject.toml. I was playing with this last month at https://github.com/bbkane/simple-python-package .

@ross
Copy link
Contributor Author

ross commented Jan 21, 2023

Then you can remove the export PYTHONPATH=.:$PYTHONPATH line in ./script/test and script/coverage

Nice. Done.

In fact, for recent (maybe too recent for octo's taste) versions of setuptools and Python, it's possible to use ONLY pyproject.toml. I was playing with this last month at https://github.com/bbkane/simple-python-package .

Will explore that, at some point but not in this PR/thread. I have looked at, and not liked, the newer setuptools replacement stuff like poetry. If at some point it's possible to replace setup.py itself and stick with the tooling I wouldn't complain. There's definitely times when it'd be nice to be able to process config rather than have to run setup.py to get answers (e.g. ./script/update-requirements.) I assume the pyproject.toml stuff is mostly just moving stuff out of setup.cfg there...

@ross ross changed the title Explor using isort to manage imports Explore using isort to manage imports Jan 22, 2023
@bkane-msft
Copy link
Contributor

Sorry, I didn't get notified of your reply for some reason.

I also don't like the setuptools replacement tooling - I feel it's not needed for packaging pure Python code. Fortunately, it's possible to just used setuptools and build with a pyproject.toml-based build system. I haven't tested this, but octodns-ns1 already depends on build so I don't think a setup.py -> pyproject.toml migration would require any requirements-dev.txt changes (other than version upgrades maybe). Anyhoo, as you said, this is for another PR/thread :)

@bkane-msft
Copy link
Contributor

I'll try to make an experimental PR just to see if CI passes for all Python versions after this PR gets merged.

@bkane-msft
Copy link
Contributor

So I started playing with the "migrate everything to pyproject.toml but still use setuptools/build packages" idea and it turns out that relies on some beta functionality of setuptools:

* Getting build dependencies for sdist...
/private/var/folders/cv/pmplkw8n30lby2tc0j84lt1m000yft/T/build-env-c1qb36cr/lib/python3.10/site-packages/setuptools/config/pyprojecttoml.py:108: _BetaConfiguration: Support for `[tool.setuptools]` in `pyproject.toml` is still *beta*.  
  warnings.warn(msg, _BetaConfiguration)

I've commented in pypa/setuptools#3632 and subscribed to pypa/setuptools#3683 and pypa/setuptools#3347 in hopes that someone will mention that the functionality is stabilizing. Once/If it stabilizes, I'll bring this back idea back up, but for now it's probably not worth its own issue/PR to track

@ross
Copy link
Contributor Author

ross commented Feb 10, 2023

Thanks for digging into it! Will keep an eye on those as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants