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

Add legacy support (Python 3.7 & 3.8) in Sterzo #27

Merged
merged 6 commits into from
Nov 19, 2023
Merged

Add legacy support (Python 3.7 & 3.8) in Sterzo #27

merged 6 commits into from
Nov 19, 2023

Conversation

tensorturtle
Copy link
Contributor

importlib.resources.files in sterzo.py was newly introduced in Python 3.9.
I needed to use this library on Python 3.8, so I implemented a version check to use the correct importlib API. More info at python docs

The only side effect is that the sterzo-challenge-codes.dat file had to be moved out of the data directory.

@tensorturtle
Copy link
Contributor Author

I tested the change on Python 3.8 and Python 3.10 with success.

Copy link
Owner

@zacharyedwardbull zacharyedwardbull left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for spotting this lack of compatibility with Python 3.7/3.8 and suggesting this fix. It looks good, I've just left a few comments.

fp.seek(self._latest_challenge * 2, 1)
code_1 = int.from_bytes(fp.read(1), 'little')
code_2 = int.from_bytes(fp.read(1), 'little')
# moved .dat file out of 'data' because accessing directories not possible for importlib.resources.path

Choose a reason for hiding this comment

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

I think we can keep the data directory, we just need to add __init__.py to make it a Python package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean putting a __init__.py to the data directory like so?

├── pycycling
       ├── sterzo.py
       ├── data
       │   ├── __init__.py
       │   └── sterzo-challenge-codes.dat

Choose a reason for hiding this comment

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

Yep! Can be completely empty I believe.

code_1 = int.from_bytes(fp.read(1), 'little')
code_2 = int.from_bytes(fp.read(1), 'little')
else: # legacy support < 3.9
with importlib.resources.path(__package__, 'sterzo-challenge-codes.dat') as file_path:

Choose a reason for hiding this comment

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

Is there a reason not to use importlib.resources.open_binary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be using that instead.

else: # legacy support < 3.9
with importlib.resources.path(__package__, 'sterzo-challenge-codes.dat') as file_path:
with open(file_path, 'rb') as fp:
fp.seek(self._latest_challenge * 2, 1)

Choose a reason for hiding this comment

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

Can we refactor this to remove the duplication of these lines?

@tensorturtle
Copy link
Contributor Author

Putting sterzo-challenges-codes.dat into a data directory with __init__.py does not work:

Using:

with importlib.resources.open_binary(__package__, 'data/sterzo-challenge-codes.dat') as fp:

Throws:

  File "/home/tensorturtle/Repos/pycycling/pycycling/sterzo.py", line 34, in _activate_steering_measurements
    with importlib.resources.open_binary(__package__, 'data/sterzo-challenge-codes.dat') as fp:
  File "/usr/lib/python3.10/importlib/resources.py", line 42, in open_binary
    resource = _common.normalize_path(resource)
  File "/usr/lib/python3.10/importlib/_common.py", line 34, in normalize_path
    raise ValueError(f'{path!r} must be only a file name')
ValueError: 'data/sterzo-challenge-codes.dat' must be only a file name

I think this limitation is what the docs are referring to when it says:

The main drawback of these functions is that they do not support directories: they assume all resources are located directly within a package.
https://docs.python.org/3/library/importlib.resources.html#deprecated-functions

@zacharyedwardbull
Copy link
Owner

Since we've created a subpackage, you need to add a import pycycling.data to the imports then replace __package__ with pycycling.data in the call.

@tensorturtle
Copy link
Contributor Author

Thank you for guiding me. Please see latest commit

@tensorturtle
Copy link
Contributor Author

I tested both branches

@zacharyedwardbull zacharyedwardbull merged commit cc230d7 into zacharyedwardbull:master Nov 19, 2023
5 checks passed
@tensorturtle
Copy link
Contributor Author

Fix #28

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