-
Notifications
You must be signed in to change notification settings - Fork 3
Modernize #17
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
base: main
Are you sure you want to change the base?
Modernize #17
Conversation
|
I was so free to update the CI as well. The new CI uses https://docs.pypi.org/trusted-publishers/ I hope the modified/copied ci run through. |
Modernized version of package. Lock files should be commited to allow for easier reproduction of errors/problems.
|
Ok I tried around using the maturin in the CI but this required an update of PyO3 and chrono as it seems which is more as my current rust knowledge allows for. I paste my WIP maybe you can work on it faster than I do? |
|
Ok I managed to build everything but Windows aarch64 platform (maybe we can skip it?). I hope it is a good enough start point for you release a new version. There are some warnings when building that might be good to fix? Also there are some crono feature flags in pyo3 that might be good to have look at? But maybe release just a new build for a new python version and optimize later? |
|
@gukoff could you have a look? |
|
Thanks a lot @CarliJoy, I appreciate it! I will review it in the next few days. |
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.
Pull Request Overview
This PR modernizes the Python package build system by migrating from setuptools to Maturin with pyproject.toml configuration. The changes update the build infrastructure to use modern Python packaging standards and implement a comprehensive CI pipeline for multiple Python versions and platforms.
- Migrated from setup.py to pyproject.toml with Maturin as the build backend
- Updated PyO3 from version 0.13.1 to 0.25.1 and modernized Rust code structure
- Implemented comprehensive CI workflow supporting Python 3.9-3.13 across multiple platforms and architectures
Reviewed Changes
Copilot reviewed 7 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Updated PyO3 bindings to use modern function decorators and module structure |
| setup.py | Removed legacy setuptools configuration |
| setup.cfg | Removed old configuration in favor of pyproject.toml |
| pyproject.toml | Added modern Python project configuration with Maturin build system |
| MANIFEST.in | Added manifest for source distribution inclusion |
| Cargo.toml | Updated PyO3 version and library name configuration |
| .github/workflows/ci.yml | Comprehensive CI pipeline with multi-platform builds and modern tooling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| dt.hour() as u8, | ||
| dt.minute() as u8, | ||
| dt.second() as u8, | ||
| microsecond, |
Copilot
AI
Oct 1, 2025
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.
The microsecond parameter type has changed from u32 to the inferred type from nanosecond() / 1000. Since nanosecond() returns u32 and the division result is u32, but PyDateTime::new expects u32, this should be explicitly cast to ensure type safety: microsecond as u32.
| microsecond, | |
| microsecond as u32, |
| authors = ["Konstantin Gukov <gukkos@gmail.com>"] | ||
| name = "python-dtparse" | ||
| version = "1.3.2" | ||
| edition = "2021" |
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.
Does this line have an effect?
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 don't remember.
I think the build failed without but I am not sure.
|
|
||
| - run: uv run pytest | ||
|
|
||
| # https://github.com/marketplace/actions/alls-green#why used for branch protection checks |
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.
Note to myself: add check as a branch protection policy
use pyproject.toml and lock files
Modernized version of package.
Lock files should be commited to allow for easier reproduction of errors/problems.
CI for new python versions
adopted from https://github.com/samuelcolvin/rtoml/blob/a957d04c4a32de00a5901a9dd78df559b7cd901e/.github/workflows/ci.yml
fixes #16