-
Notifications
You must be signed in to change notification settings - Fork 173
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
Migrate to Poetry #891
Migrate to Poetry #891
Conversation
f7d0ad6
to
eb9b41e
Compare
ba56db2
to
8eb0edb
Compare
This is declared in both an extra and a main dependency in the status quo setup.py: * https://github.com/reddit/baseplate.py/blob/377488cf6ca9329697f561628e2bb128296e82aa/setup.py#L13 * https://github.com/reddit/baseplate.py/blob/377488cf6ca9329697f561628e2bb128296e82aa/setup.py#L49 We should make it just a main dependency.
@@ -1,3 +1,99 @@ | |||
[tool.poetry] | |||
name = "baseplate" | |||
version = "2.7.0a1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notable change: right now, the version is embedded here and not based on SCM tags, so we'll need to manually update this file when making new releases.
We may want to consider adopting https://github.com/mtkennerly/poetry-dynamic-versioning (haven't used it, but looks like what we want)? I'm inclined to wait and do it in a followup PR once we're sure all the other machinery is working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NICE!
Makefile
Outdated
black baseplate/ tests/ | ||
$(REORDER_PYTHON_IMPORTS) --application-directories /tmp --exit-zero-even-if-changed $(PYTHON_EXAMPLES) | ||
black docs/ # separate so it uses its own pyproject.toml | ||
poetry run $(REORDER_PYTHON_IMPORTS) --exit-zero-even-if-changed $(PYTHON_SOURCE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With reorder-python-imports 3.12.0
, I get
reorder-python-imports: error: unrecognized arguments: --separate-from-import --separate-relative
Removal of those arguments from the Makefile line 1 allows it to run for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, how did you end up with that version of reorder-python-imports? I also ran into that issue initially but I pinned it back down to the version we're using prior to this branch since I was trying to avoid making additional changes:
Line 75 in 81544de
reorder-python-imports = "2.4.0" |
Did you install dependencies with Poetry? What does poetry show reorder-python-imports
produce?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did a pipx install reorder-python-imports
$ poetry show reorder-python-imports
name : reorder-python-imports
version : 2.4.0
description : Tool for reordering python imports
dependencies
- aspy.refactor-imports >=2.1.0
requirements.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few references to requirements.txt and runtime.txt remain in the repo. Mostly comments or markdown.
git grep 'runtime.*txt'
git grep 'requirements.*txt'
Makefile
Outdated
flake8 baseplate tests | ||
PYTHONPATH=. pylint baseplate/ | ||
mypy baseplate/ | ||
poetry run $(REORDER_PYTHON_IMPORTS) --diff-only $(PYTHON_SOURCE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into a similar issue to make fmt
for make lint
reorder-python-imports: error: unrecognized arguments: --diff-only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some nits and some other comments! But overall this is great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meant to approve last review 😅
@salomon-smekecohen I pushed a new commit addressing some of the main feedback. Let me know if you have thoughts on any of the open threads (or threads I closed)! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
king
This PR migrates Baseplate to use Poetry for dependency management. Poetry is now responsible for managing Baseplate's dependencies and setting up a local Python environment. The benefits over the previous setup include:
Poetry only affects our own development workflow; the files that are published to PyPI are effectively identical. Consumers do not need to switch to Poetry or change anything about their workflow.
Changes in this PR
This PR upgrades many dependencies, but I've held back a few (pytest, black, reorder-python-imports) which require code changes for now.
Keep in mind that these dependency bumps, just like the ones in the old
requirements*.txt
files, are only used by this repo's own tests. They do not affect consumers or releases we make.Validation
I generated a wheel on the current
develop
branch (python setup.py bdist_wheel
) and a wheel on this branch using Poetry (poetry build
). You can see a diff of the extracted wheel contents here: https://gist.github.com/chriskuehl/207271df9b590c83557d2d046b4f0913Most of the changes are due to differences in whitespace or sorting between setuptools and Poetry. The only notable change is that I removed the
all
extra since it's a little harder to support, and it appears to be unused. I don't think there's much use-case forall
other than developing on the package itself, andpoetry install --all-extras
handles that for us.