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

Migrate to poetry #144

Merged
merged 19 commits into from
Aug 4, 2021
Merged

Migrate to poetry #144

merged 19 commits into from
Aug 4, 2021

Conversation

alexmisk
Copy link
Contributor

Here is an almost working draft, I'll leave a few comments later today. You can examine a build generated by Poetry here: https://test.pypi.org/project/pyserde-test/

@alexmisk alexmisk requested a review from yukinarit July 29, 2021 06:37
@alexmisk alexmisk linked an issue Jul 29, 2021 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #144 (e342ee4) into master (2f62ec1) will not change coverage.
The diff coverage is n/a.

❗ Current head e342ee4 differs from pull request most recent head 27aa9b5. Consider uploading reports for the commit 27aa9b5 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #144   +/-   ##
=======================================
  Coverage   89.08%   89.08%           
=======================================
  Files          11       11           
  Lines        1090     1090           
  Branches      239      239           
=======================================
  Hits          971      971           
  Misses         76       76           
  Partials       43       43           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f62ec1...27aa9b5. Read the comment docs.

@alexmisk
Copy link
Contributor Author

A few notes on the changes. I've somewhat mitigate problems with Poetry so one can manage virtualenvs, dependencies, build process and publishing with it now.

Tried to fully mimic current pipenv / setup.py behavior, but faced these issues:

  • Poetry config become bloated: to support separation of dependencies both after cloning project from GitHub and installing it from PyPi we need to double-declare deps in several config sections (see Canonical way for adding development dependencies to "setup.py" file "extras_require". python-poetry/poetry#1416). Config can be trimmed though if we drop support for installing test deps via PyPi (pip install pyserde[test])
  • Pre-commit refused to pick up config for black and isort from pyproject.toml after migration, so I moved it to .pre-commit-config.yaml (but it’s for the better, actually)
  • Pre-commit minimal Python version is 3.6.1, so I had to bump it in the pyproject.toml. Poetry refuses to install deps with 3.6
  • Poetry currently has an issue with Python 3.10 (see JSONDecodeError on Python 3.10 python-poetry/poetry#4210) so I removed 3.10-dev from CI pipeline. It’s certainly a temporary thing
  • I've to remain Pipfile in examples folder untouched because tomlfile.py depends on it

Everything else seems fine. Waiting for your comments! :-)

@yukinarit
Copy link
Owner

Hi @alexmisk

Sorry for the delay!

Config can be trimmed though if we drop support for installing test deps via PyPi

I don't mind dropping pip install pyserde[test]. I don't use it, and probably nobody uses it 😄

Copy link
Owner

@yukinarit yukinarit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left 1 comment. Everything else looks good to me! 👍

CONTRIBUTING.md Outdated
- Install pipenv:
- Install Poetry (used for dependency management and packaging):

For OSX / Linux / WSL
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, the latest OS for Mac is called macOS.

@alexmisk
Copy link
Contributor Author

alexmisk commented Aug 4, 2021

Hi!

Extra deps removed, OS name changed :-) PR is ready to merge.

Yet another detail: as you may know, Poetry tracks package version in pyproject.toml and has a convenient way to change it. For example:

❯ poetry version minor
Bumping version from 0.4.0 to 0.5.0
❯ poetry version patch
Bumping version from 0.5.0 to 0.5.1

More on that: https://python-poetry.org/docs/cli/#version

So maybe there is no need to keep version number in serde/__init__.py anymore. But it's up to you, of course.

@alexmisk alexmisk marked this pull request as ready for review August 4, 2021 07:26
@alexmisk alexmisk requested a review from yukinarit August 4, 2021 07:26
@yukinarit
Copy link
Owner

LGTM! 🎉

@yukinarit yukinarit merged commit d8d6702 into yukinarit:master Aug 4, 2021
@yukinarit
Copy link
Owner

I don't mind deleting version number in serde/__init__.py.

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.

Migrate from pipenv to poetry
2 participants