Fix for py3.13+ by replacing past.basestring#1245
Fix for py3.13+ by replacing past.basestring#1245r-burns wants to merge 1 commit intodronekit:masterfrom
Conversation
The `future` package is not compatible with python 3.13+, so we need to replace any imports of the `past` module. As far as I can tell, it's only used for basestring, which is trivial to implement using the same logic the future module uses: https://github.com/PythonCharmers/python-future/blob/2d56f83adab5a0957cfc5abbe62db1e2d1912b61/src/past/types/basestring.py#L25-L26
|
How was this tested? |
|
Nothing beyond verifying that it's importable now. Behavior should be the exact same as before, this is exactly how the basestring instance check was implemented. |
peterbarker
left a comment
peterbarker
left a comment
There was a problem hiding this comment.
Please test that the new formulation actually does more than compile. Anything which crosses the mode codepath is fine.
| @mode.setter | ||
| def mode(self, v): | ||
| if isinstance(v, basestring): | ||
| if isinstance(v, (bytes, str)): |
There was a problem hiding this comment.
Shouldn't this technically be
| if isinstance(v, (bytes, str)): | |
| if isinstance(v, (unicode, str)): |
?
Seriously, just:
| if isinstance(v, (bytes, str)): | |
| if isinstance(v, str): |
... we don't need Python2 compatibility any more (pymavlink is no longer Python2-compliant so no new installs are going to work on Py2 anyway).
There was a problem hiding this comment.
I don't think unicode exists in python3, at least by default. My understanding is that it's a python2 forward-compat alias for a python3 str.
I also think code that has historically supported python2 may still be passing bytes around as strings, so that's why the past module uses (bytes, str) instead of just str. If that's not the case here for dronekit, then I agree it's simpler to just use str.
|
To elaborate a bit more on the lack of testing, I'm just packaging this for a distro so I don't really know how to use this package and test it myself. I'm just packaging it up and have noticed that it doesn't import on py313 unless I make some changes. I'd typically try to run unit tests but I don't see any here. Happy to do some actual functionality testing if you have any suggestions. |
2 participants
The
futurepackage is not compatible with python 3.13+, so we need to replace any imports of thepastmodule.As far as I can tell, it's only used for basestring, which is trivial to implement using the same logic the future module uses: https://github.com/PythonCharmers/python-future/blob/2d56f83adab5a0957cfc5abbe62db1e2d1912b61/src/past/types/basestring.py#L25-L26