Fix importing adafruit_requests in CPython
#94
Conversation
| @@ -37,35 +37,117 @@ | |||
| __repo__ = "https://github.com./adafruit/Adafruit_CircuitPython_Requests.git" | |||
|
|
|||
| import errno | |||
| import sys | |||
|
|
|||
| if sys.implementation.name == "circuitpython": | |||
There was a problem hiding this comment.
Previously this pattern was used to only run the typing-related code in Python implementations which support it (such as CPython):
try:
# typing-related code here
...
except ImportError:
passBut that caused the CPython import failure issue described in #93 to be swallowed, so to help make debugging similar issues easier in the future, I replaced that pattern with this approach of reading sys.implementation.name to check if CircuitPython is being used or not instead.
| class InterfaceType(Protocol): | ||
| """Describes the structure every interface type must have.""" | ||
|
|
||
| @property | ||
| def TLS_MODE(self) -> int: # pylint: disable=invalid-name | ||
| """Constant representing that a socket's connection mode is TLS.""" | ||
| ... |
There was a problem hiding this comment.
I discovered CircuitPython does not appear to support class variables, because previously I wrote this Protocol class using this more concise form:
class InterfaceType(Protocol):
"""Describes the structure every interface type must have."""
TLS_MODE: intbut that resulted in a SyntaxError: invalid syntax exception being raised for the TLS_MODE: int line.
Luckily Protocol supports an alternative syntax for these kinds of variables (specifically for Python 2.7-3.5) using property which I was able to leverage here.
| @@ -25,7 +25,7 @@ | |||
| # Uncomment the below if you use native CircuitPython modules such as | |||
| # digitalio, micropython and busio. List the modules you use. Without it, the | |||
| # autodoc module docs will fail to generate with a warning. | |||
| autodoc_mock_imports = ["adafruit_esp32spi", "adafruit_wiznet5k", "adafruit_fona"] | |||
| # autodoc_mock_imports = ["digitalio", "busio"] | |||
There was a problem hiding this comment.
Since we no longer use those imports, I put back what this was before #87.
| envlist = py37 | ||
|
|
||
| [testenv] | ||
| changedir = {toxinidir}/tests |
There was a problem hiding this comment.
I included this changedir so that we run the tests from the tests/ folder to help ensure that adafruit_requests gets imported from the installed package in tox's virtual environment instead of being imported locally from the root of the project folder. I believe that should result in the tests importing adafruit_requests in a manner more representative of how CPython users would if they installed the package via pip.
| @@ -157,7 +239,7 @@ def _recv_into(self, buf: bytearray, size: int = 0) -> int: | |||
| read_size = len(b) | |||
| buf[:read_size] = b | |||
| return read_size | |||
| return self.socket.recv_into(buf, size) | |||
| return cast("SupportsRecvInto", self.socket).recv_into(buf, size) | |||
There was a problem hiding this comment.
This cast() is needed for mypy to understand that we can safely call recv_into() on self.socket here because the legacy type of CircuitPython socket (represented by the LegacyCircuitPythonSocketType and the self._backwards_compatible flag checked above being True) does not have a recv_into() method.
There was a problem hiding this comment.
@kevincon it seems that the change here caused the library to stop working on microcontrollers with this error:
code.py output:
Connecting to AP...
Connected to WBurn-2G RSSI: -50
Fetching text from http://wifitest.adafruit.com/testwifi/index.html
Traceback (most recent call last):
File "code.py", line 58, in <module>
File "adafruit_requests.py", line 847, in get
File "adafruit_requests.py", line 719, in request
File "adafruit_requests.py", line 210, in __init__
File "adafruit_requests.py", line 297, in _readto
File "adafruit_requests.py", line 241, in _recv_into
TypeError: function takes 2 positional arguments but 3 were given
Code done running.
I think we'll need to revert it for now. Curious if you have any ideas that could make it worth in both environments?
There was a problem hiding this comment.
Actually maybe this is not what is causing the error. My mistake. I reverted it in my local copy, but still seeing that error
| @@ -440,7 +522,7 @@ def _free_sockets(self) -> None: | |||
|
|
|||
| def _get_socket( | |||
| self, host: str, port: int, proto: str, *, timeout: float = 1 | |||
| ) -> SocketType: | |||
| ) -> CircuitPythonSocketType: | |||
There was a problem hiding this comment.
This change was needed because the recv_into() method is called on the socket returned by this _get_socket() method in the request() method, and not every SocketType provides a recv_into() method (specifically LegacyCircuitPythonSocketType does not). CircuitPythonSocketType meets that requirement while also being a valid SocketType to support the socket being passed to other methods which require a SocketType socket.
|
I successfully tested the version from this branch on PyPortal and CPython with the simpletest scripts in the repo. The changes you've made and reasoning for them make sense to me. Thanks again for working on this. |
Updating https://github.com./adafruit/Adafruit_CircuitPython_DRV2605 to 1.2.0 from 1.1.13: > Merge pull request adafruit/Adafruit_CircuitPython_DRV2605#29 from tekktrik/doc/typing-and-documentation > Merge pull request adafruit/Adafruit_CircuitPython_DRV2605#28 from tekktrik/feature/add-eight-slot > update rtd py version Updating https://github.com./adafruit/Adafruit_CircuitPython_HTS221 to 1.1.7 from 1.1.6: > Merge pull request adafruit/Adafruit_CircuitPython_HTS221#10 from tekktrik/feature/add-typing > update rtd py version Updating https://github.com./adafruit/Adafruit_CircuitPython_PCA9685 to 3.4.0 from 3.3.9: > Merge pull request adafruit/Adafruit_CircuitPython_PCA9685#42 from tekktrik/doc/add-typing > Merge pull request adafruit/Adafruit_CircuitPython_PCA9685#43 from tekktrik/feature/add-mode2-reg > update rtd py version Updating https://github.com./adafruit/Adafruit_CircuitPython_Wiznet5k to 1.12.0 from 1.11.2: > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#49 from AdamCummick/main Updating https://github.com./adafruit/Adafruit_CircuitPython_AVRprog to 1.4.2 from 1.4.1: > Merge pull request adafruit/Adafruit_CircuitPython_AVRprog#31 from aarontusko/fix-write_fuses-busy_wait Updating https://github.com./adafruit/Adafruit_CircuitPython_OneWire to 1.2.8 from 1.2.7: > Merge pull request adafruit/Adafruit_CircuitPython_OneWire#24 from tekktrik/doc/add-typing > update rtd py version Updating https://github.com./adafruit/Adafruit_CircuitPython_PIOASM to 0.5.4 from 0.5.3: > Merge pull request adafruit/Adafruit_CircuitPython_PIOASM#34 from dannystaple/mov_operators Updating https://github.com./adafruit/Adafruit_CircuitPython_Requests to 1.10.5 from 1.10.4: > Merge pull request adafruit/Adafruit_CircuitPython_Requests#94 from kevincon/fix-93 Updating https://github.com./adafruit/Adafruit_CircuitPython_Waveform to 1.3.9 from 1.3.8: > update rtd py version
|
This is great! The only question I have is how often this library gets used on CPython w/ Blinka if the CPython |
|
Never mind, I caught up on the relevant Issue. Sounds like a cool project! |
This PR fixes importing
adafruit_requestsin CPython by replacing the imports of various classes fromadafruit_esp32spi,adafruit_wiznet5k, etc. (which weren't specified as requirements ofadafruit_requestsand thus caused importingadafruit_requeststo fail due to not being available) with protocols that describe the common structure of those types.To help prevent the use of
adafruit_requestsin CPython from breaking in the future, this PR also adds a step to the Build CI job which uses tox to build the package and run its tests in a virtual environment.I also tested the changes to
adafruit_requests.pyin CircuitPython by copying the file to thelib/folder of my MagTag device running the COVID tracking project and verifying that the display showed the COVID stats as expected (although note that project's data source has stopped reporting new data since March 7, 2021).Fixes #93.