Skip to content

Fixes #220 for platforms that do not support typing #222

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

Closed
wants to merge 1 commit into from

Conversation

cdwilson
Copy link
Contributor

@cdwilson cdwilson commented Mar 1, 2022

This addresses the issue reported in #220 where the typing module is not supported on MicroPython.

@cdwilson cdwilson marked this pull request as draft March 1, 2022 22:16
@cdwilson
Copy link
Contributor Author

cdwilson commented Mar 1, 2022

@Neradoc if you are able, can you check this out and let me know if this works for you?

I tested this out myself using MicroPython 1.18 and I don't see any import errors, but it would be good to get someone else to test it out.

@ladyada ladyada requested a review from dhalbert March 2, 2022 03:49
@ladyada
Copy link
Member

ladyada commented Mar 2, 2022

can ya rebase plz :)

@cdwilson
Copy link
Contributor Author

cdwilson commented Mar 2, 2022

can ya rebase plz :)

@ladyada sorry, can you clarify what you'd like me to rebase this off of? (it's currently rebased off main)

@cdwilson
Copy link
Contributor Author

cdwilson commented Mar 2, 2022

Also, I'm not quite sure what to do about the failing CI check:

    from __future__ import annotations
    ^
SyntaxError: from __future__ imports must occur at the beginning of the file

AFAIK, MicroPython doesn't support the __future__ module:

MPY: sync filesystems
MPY: soft reboot
Traceback (most recent call last):
  File "main.py", line 7, in <module>
  File "busio.py", line 16, in <module>
  File "adafruit_platformdetect/__init__.py", line 14, in <module>
  File "adafruit_platformdetect/board.py", line 24, in <module>
ImportError: no module named '__future__'
MicroPython v1.18-168-gd3bcb09d7 on 2022-03-01; SparkFun STM32 MicroMod Processor with STM32F405RG
Type "help()" for more information.
>>> 

So I tried wrapping it in a try/except block:

try:
    from __future__ import annotations
except ImportError:
    pass

This seems to work fine for MicroPython in my initial tests, but also seems to be discouraged in PEP-236:

try/except is a runtime feature; future_statements are primarily compile-time gimmicks, and your try/except happens long after the compiler is done. That is, by the time you do try/except, the semantics in effect for the module are already a done deal. Since the try/except wouldn't accomplish what it looks like it should accomplish, it's simply not allowed. We also want to keep these special statements very easy to find and to recognize.

Any suggestions for how to fix this?

import os
import sys
from typing import Optional, TYPE_CHECKING

# from tokenize import Name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

MicroPython doesn't support the tokenize module, so I had to comment it out to get this working. It appears to be an unused import and can probably be removed, but I wanted to double check that I didn't miss something.

@Neradoc
Copy link

Neradoc commented Mar 2, 2022

Tested in Micropython, it allows importing the blinka modules.

Also, I'm not quite sure what to do about the failing CI check:

    from __future__ import annotations
    ^
SyntaxError: from __future__ imports must occur at the beginning of the file

Note that this is an error in C Python in general, not just the docs CI.

__future__ is used here to avoid an issue with the typing annotations that would otherwise require a circular import.

  • board.py uses the Detector type from __init__ (but does not need the import outside of typing)
  • The Detector type uses the Board type from board.py (which is imported)

It is possible to use instead a parent "protocol" class for Detector for the typing annotation in a common file and completely drop the __future__ import. A combination of from .types import DetectorProtocol and class Detector(DetectorProtocol):.

@dhalbert
Copy link
Contributor

dhalbert commented Mar 4, 2022

Note that any general-purpose annotation types, protocols, etc. should now be added to https://github.com/adafruit/Adafruit_CircuitPython_Typing. I see what you're adding is specific to Blinka: those I would not expect to see in https://github.com/adafruit/Adafruit_CircuitPython_Typing.

@cdwilson
Copy link
Contributor Author

cdwilson commented Mar 4, 2022

It is possible to use instead a parent "protocol" class for Detector for the typing annotation in a common file and completely drop the future import. A combination of from .types import DetectorProtocol and class Detector(DetectorProtocol):.

@Neradoc I'm not totally sure I understood what you were proposing, but I pushed up some changes that I think do what you suggested. (This is my first time doing anything with python typing, so let me know if I'm doing something totally wacky.)

BTW, it looks like Protocol was introduced in python 3.8 and the CI is using python 3.7 so it's failing. I think the goal for this package was is to support python 3.6+, so this approach may not be suitable? Open to suggestions/feedback.

Comment on lines 37 to 40
__version__ = "0.0.0-auto.0"
__repo__ = "https://github.com/adafruit/Adafruit_CircuitPython_PlatformDetect.git"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dhalbert or @makermelissa can you give me some context as to why these lines are included in __init__.py, board.py, and chip.py (vs. just having it in __init__.py)? Just wondering if these should be added to any new files I add (like protocols.py that I added in this draft)

@dhalbert
Copy link
Contributor

dhalbert commented Mar 4, 2022

BTW, it looks like Protocol was introduced in python 3.8 and the CI is using python 3.7 so it's failing. I think the goal for this package was is to support python 3.6+, so this approach may not be suitable? Open to suggestions/feedback.

You can use typing_extensions for this, which backports Protocols to 3.7. See https://github.com/adafruit/Adafruit_CircuitPython_Typing/blob/main/circuitpython_typing/__init__.py#L20-L24.

@makermelissa
Copy link
Collaborator

Closing in favor of #230, which fixed the error.

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.

5 participants