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

Py313 #27

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Py313 #27

wants to merge 2 commits into from

Conversation

debauer
Copy link

@debauer debauer commented Jan 23, 2025

Hello just want to drop some love. Thanks for your work. Thats completed my Pipeline :-)
But i wanted to use a modern Python and also some tools i normaly use.
Have a look and drop your wishes.

* added support for python up to 3.13
* dropped support for python3.9
* fixed pyproject.toml for poetry >= 2.0
* moved modules in src folder
* added service and g2s as scripts in poetry.toml
* removed taskipy (poetry can do that)
* wrapped a small script around gunicorn
* replaces black with ruff
* added some rudimentary configs for mypy
* added scripts for ci, works right now only on gitlab. Can add for github, but normaly i don't maintain my code on github anymore.

I think i missed something.

After poetry install g2s and service command works as intended.
How about publish it on pypi?

* added support for python up to 3.13
* dropped support for python3.9
* fixed pyporject.toml for poetry >= 2.0
* moved modules in src folder
* added service and g2s as scripts in poetry.toml
* removed taskipy (poetry can do that)
* wrapped a small script around gunicorn
@DeflateAwning
Copy link
Contributor

This should be several separate PRs if you want someone to review it. Reviewing a PR that has changes and runs an autoformatter is very painfail, and conflates the changes.

Copy link
Owner

@kirberich kirberich left a comment

Choose a reason for hiding this comment

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

Thanks for this - as @DeflateAwning said, I won't be able to properly review/merge this because it's too many separate things at once, but I did have a quick look and added a few notes around the python version update and a few other bits

There's lots of good stuff here, if you can split up the various improvements into separate PRs I'll be more than happy to give them a proper review!

It might be a good idea to pull the latest code, as I've just merged a few PRs and also added ruff as a proper dependencies, as well as ran the formatter for some old files I hadn't touched and reformatted since starting to use ruff

@@ -2,10 +2,11 @@
Simple python script for converting gerber files into a 3d printable solder stencil scad file

## Installation
gerber to scad requires python3.9
gerber to scad requires >=python3.10
Copy link
Owner

Choose a reason for hiding this comment

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

I'm happy in general to update python but it really would have to be its own PR, and the dockerfile would need updating as well - I'd need this to be a separate change to make sure the deployment still works and make fixing it easy if it breaks the deployment.

If we are going to update python though, let's just require python 3.13 right away, as I don't really want to have to test it with older versions and people might get confused why it doesn't work if we start using any 3.13 features.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest that Python 3.10 is a good middleground atm. It's the default many distros ship with, and it's the first version that supports 98% of modern type checking syntax. The big change from Python 3.9 is that it supports the modern int | list[int] syntax (instead of Union[int, list[int]]).

Python 3.13 is quite new. With tools like pipx or uvx, not a big deal to run with that pinned Python version, but might pose a challenge for some.

@@ -0,0 +1,22 @@
#!/bin/bash
Copy link
Owner

Choose a reason for hiding this comment

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

I may be missing something but I'm not sure this script is needed - does it do anything that ruff format . doesn't do? (other than also running ruff check, which I'd prefer to be separate really


EXIT_CODE=0

poetry run ruff check --select I001,I002 "${ruff_fix[@]}" "${files[@]}" || EXIT_CODE=1
Copy link
Owner

Choose a reason for hiding this comment

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

If it makes sense to include/exclude specific formatting rules, let's set them in pyproject.yaml, not adhoc here

args=("." "$@")
fi

poetry run ruff check "${args[@]}"
Copy link
Owner

Choose a reason for hiding this comment

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

Same question as above, ruff check is already recursive, does this script add anything?


set -euo pipefail

poetry run mypy \
Copy link
Owner

Choose a reason for hiding this comment

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

This project uses pyright for types - though I should probably make that clear in the readme and add it as an explicit dependency, as right now it just works when using vscode

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.

3 participants