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

Standardise on one approach to numeric parsing, drop Long support from Python 2.x #98

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

Conversation

Gadgetoid
Copy link
Contributor

Previously there was a mix of define-based polyfill and explicit version checking.

This change canonicalizes upon the latter, always using PyInt under Python 2.x and PyLong under 3.x.

As such this change will remove implicit compatibility with Long datatypes from the Python 2.x version of this library.

Since this (possibly unintended) compatibility witgh Python 2.x long types led to unexpected behaviour (see #34) it seems sensible to remove it and favour a TypeError.

Additionally, SPI doesn't support the transmission of any one value that might be represented by a Long in Python 2.x so this support was redundant.

I have also included a new folder of tests which run under both Python 2.x and Python 3.x. These are intended to validate (some not all) functionality of the spidev library and help avoid regressions (such as the one I introduced in #93 and discuss in #97). The included README.md documents, somewhat, how to run these properly.

This PR is raised as a draft because it's a bit meaty and potentially controversial. These tests are also incomplete and unable to run in a CI environment- that's something we should look into fixing.

Previously there was a mix of define-based polyfill and explicit version checking.

This change canonicalizes upon the latter, *always* using PyInt under Python 2.x and PyLong under 3.x.

As such this change will remove implicit compatibility with Long datatypes from the Python 2.x version of this library.

Since this (possibly unintended) compatibility witgh Python 2.x long types led to unexpected behaviour (see doceme#34) it seems sensible to remove it and favour a TypeError.

Additionally, SPI doesn't support the transmission of any one value that might be represented by a Long in Python 2.x so this support was redundant.
@Gadgetoid
Copy link
Contributor Author

@linuxtim since you've raised a ... relatively ... recent PR, I'd appreciate feedback on these changes if you have time.

It would be nice to move py_spidev forwards.

@doceme are you still out there? Would you consider transferring ownership of this repository to @pimoroni so I can deal with PRs and issues as they come and try to find some more maintainers?

@linuxtim
Copy link

linuxtim commented Nov 3, 2023

The only thing I feel qualified to say on this is that anyone using Python 2.x at this stage is unlikely to be inconvenienced by this change - in the event that merging this PR caused an incompatibility for them they are likely to be able to continue to use a previous release of py-spidev. That is to say that the Python 2.x ecosystem is pretty much stationary at this point, and many (most?) libraries have dropped 2.x support entirely, so they are unlikely to have any dependency problems which force them to use such a new release.

@Gadgetoid
Copy link
Contributor Author

I am not averse to simply dropping all Python 2.x support, and bumping the required version in setup.py up to 3.x. That would ensure that 2.x users get an older, compatible package and we don't have to worry about legacy junk anymore.

I think embedded is weird, though, there are still esoteric sysfs GPIO and Python 2.7 setups in use.

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.

2 participants