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

Repackage to hatch/pyproject.toml #31

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

Conversation

Gadgetoid
Copy link
Collaborator

@Gadgetoid Gadgetoid commented Nov 2, 2023

This set of changes brings gpio up to the latest standard (or at least the boilerplate I work with) of Python packaging, and includes conveniences like code spellcheck, import sorting and ruff linting in order to maintain code quality and catch bugs early.

TODO

Notes

It looks like Python 2.7 support is probably not feasible against newer packaging, we should probably add the last couple of bug fixes in #26 to the 2.7-compatible 1.x.x branch.

sysfs gpio has been obsolete since 2016 and removed from kernels since 2020, with it limping on in forks of Linux mainline - https://lore.kernel.org/all/1445502750-22672-7-git-send-email-linus.walleij@linaro.org/

It's likely to be dropped from Raspberry Pi OS, too, so significant efforts to maintain this library will probably be redundant.

It's sensible to fixup the 1.0.0 branch for Python 2.7 and then update the packaging and release a 2.0.0 branch for Python 3.7+, with the implication that the 1.0.0 branch is a critical bugfix only legacy branch.

I appreciate legacy Python versions and sysfs gpio probably go hand-in-hand on more esoteric embedded devices, which is why I don't want to break the Python 2.7 library.

@Gadgetoid Gadgetoid force-pushed the repackage branch 2 times, most recently from f532691 to 4b71426 Compare November 2, 2023 13:07
pyproject.toml Outdated
requires-python = ">= 3.7"
authors = [
{ name = "Philip Howard", email = "phil@pimoroni.com" },
{ name = "Garrett Berg" },
Copy link
Owner

Choose a reason for hiding this comment

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

email vitiral@gmail.com


dev-deps:
python3 -m pip install -r requirements-dev.txt
sudo apt install dos2unix
Copy link
Owner

Choose a reason for hiding this comment

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

typically this kind of thing should NOT be in a makefile. Where is this even used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Say what? You can put whatever you want in a Makefile 🤣

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or, to more specifically answer your question, where do you put this kind of thing?

This is used in the GitHub workflows to avoid encoding common patterns in GitHub's nonsense yml format. Plus it also makes most of the things you'd want to do during development a make x away.

The alternative (and a common CI pattern) is to use a shell script, but that has its own unique set of warts and is obnoxiously painful to invoke on a local machine (source ci/tools.sh && run_some_ci_action vs just make run_some_ci_action),

This is a pattern I use across all my libraries so I don't have to know or care how to invoke a bunch of arcane commands. Though, granted, sudo apt install dos2unix is very Debian-linux specific which is great for GitHub actions but not so great for hypothetical macOS using developers aaannnd I'm totally not writing this from a mac now 👀

Copy link
Owner

Choose a reason for hiding this comment

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

I would typically put it in you developer setup documentation. I won't block the CL on this

{ name = "Garrett Berg", email = "vitiral@gmail.com" },
]
maintainers = [
{ name = "Philip Howard", email = "phil@pimoroni.com" },
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 still reviewing CL's so I think I can be added here no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will make the change, it’s pulled over from the boilerplate so it’s more an oversight than a deliberate attempt to erase you 🤣

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!

@vitiral
Copy link
Owner

vitiral commented Jun 17, 2024

LGTM

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