Skip to content

Fix typing errors for MicroPython #230

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

Merged
merged 7 commits into from
Apr 4, 2022
Merged

Conversation

makermelissa
Copy link
Collaborator

Fixes #220. Tested on Raspberry Pi Pico with latest MicroPython.

@makermelissa makermelissa requested a review from a team March 28, 2022 22:54
@makermelissa
Copy link
Collaborator Author

Ok... weird error.

black....................................................................Failed
- hook id: black
- exit code: 1

Traceback (most recent call last):
  File "/home/runner/.cache/pre-commit/repois0o6nr8/py_env-python3/bin/black", line 8, in <module>
    sys.exit(patched_main())
  File "/home/runner/.cache/pre-commit/repois0o6nr8/py_env-python3/lib/python3.7/site-packages/black/__init__.py", line 6606, in patched_main
    patch_click()
  File "/home/runner/.cache/pre-commit/repois0o6nr8/py_env-python3/lib/python3.7/site-packages/black/__init__.py", line 6595, in patch_click
    from click import _unicodefun  # type: ignore
ImportError: cannot import name '_unicodefun' from 'click' (/home/runner/.cache/pre-commit/repois0o6nr8/py_env-python3/lib/python3.7/site-packages/click/__init__.py)

@dhalbert
Copy link
Contributor

See extensive discussion in #circuitpython-dev. Black and click need to be moved up to the latest versions.

@makermelissa
Copy link
Collaborator Author

Hmm, this may be problematic:

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

This requirement pretty much breaks what the PR fixes.

@makermelissa
Copy link
Collaborator Author

makermelissa commented Mar 28, 2022

Testing on Raspberry Pi, enclosing from __future__ imports in a try/except block also results in an error. Back to the drawing board on this one.

@dhalbert
Copy link
Contributor

Hmm, this may be problematic:

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

This requirement pretty much breaks what the PR fixes.

Yes, this is why I added from __future__ import annotations to CircuitPython. I guess there could be two versions of what to import.

@dhalbert
Copy link
Contributor

The only reason to include from __future__ import annotations is to support Python 3.6. Perhaps we should stop supporting that. It breaks Jetson; do you know what else it breaks?

@makermelissa
Copy link
Collaborator Author

The only reason to include from __future__ import annotations is to support Python 3.6. Perhaps we should stop supporting that. It breaks Jetson; do you know what else it breaks?

Hmm, let me test without it

@makermelissa
Copy link
Collaborator Author

Ok, everything seems to work without it and I'm ok with not having it since we require 3.7 anyways.

@makermelissa
Copy link
Collaborator Author

Importing Detector for typing purposes was proving to be problematic without the futures line, so I just removed it. Just a normal detector import (without the TYPE_CHECKING line) caused a circular import issue anyways.

@makermelissa
Copy link
Collaborator Author

makermelissa commented Mar 28, 2022

Sweet, so this was tested on MicroPython, Linux Raspberry Pi, and passed all the the checks.

@dhalbert
Copy link
Contributor

I think the Jetson guides will need to describe how to install Python 3.7 or later for user-only use (not updating the system Python). I would expect this is possible. https://forums.developer.nvidia.com/t/python-3-6-approaching-end-of-life/194961 and other places in the that forum discuss using later versions of Python (some successful, some not).

@makermelissa
Copy link
Collaborator Author

makermelissa commented Mar 29, 2022

I think the Jetson guides will need to describe how to install Python 3.7 or later for user-only use (not updating the system Python). I would expect this is possible. https://forums.developer.nvidia.com/t/python-3-6-approaching-end-of-life/194961 and other places in the that forum discuss using later versions of Python (some successful, some not).

Ok, I'll take a look tomorrow. This PR mainly is for fixing usage on MicroPython which does not work at all at this point in time.

@tekktrik
Copy link
Member

tekktrik commented Mar 29, 2022

Can you add the from . import XXXXX to the try/except and escape it in quotes, like "Detector"?

@tekktrik
Copy link
Member

Ah, never mind, sounds like it was causing problems, never mind.

@makermelissa
Copy link
Collaborator Author

Can you add the from . import XXXXX to the try/except and escape it in quotes, like "Detector"?

It might be possible to use quote on the type name so that an import isn't necessary.

@makermelissa
Copy link
Collaborator Author

I think the Jetson guides will need to describe how to install Python 3.7 or later for user-only use (not updating the system Python). I would expect this is possible. https://forums.developer.nvidia.com/t/python-3-6-approaching-end-of-life/194961 and other places in the that forum discuss using later versions of Python (some successful, some not).

Done. See https://learn.adafruit.com/circuitpython-libraries-on-linux-and-the-nvidia-jetson-nano/initial-setup#install-python-3-dot-7-and-make-default-3114929-19.

@makermelissa
Copy link
Collaborator Author

Is this waiting on something from me?

@tekktrik
Copy link
Member

tekktrik commented Apr 4, 2022

Not sure if @dhalbert had any followup about __futures__, but looking at this now, but you could do the TYPE_CHECKING trick for importing detectior in the try/except I think if you really wanted to keep it. But with argnames like detector I think it's clearly documented either way haha.

@makermelissa
Copy link
Collaborator Author

makermelissa commented Apr 4, 2022

I think I want to leave it out because it's really touchy when trying to get it working all with MicroPython, CPython, and PyLint.

Copy link
Member

@tekktrik tekktrik left a comment

Choose a reason for hiding this comment

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

Sounds good! In that case, the typing looks good!

@makermelissa makermelissa merged commit 7af3af8 into adafruit:main Apr 4, 2022
@makermelissa
Copy link
Collaborator Author

Thanks.

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.

Typing module not available on Micropython
3 participants