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 typing #23

Merged
merged 18 commits into from
Jul 31, 2023
Merged

Add typing #23

merged 18 commits into from
Jul 31, 2023

Conversation

BiffoBear
Copy link

closes #17 Added typing. Sphinx compiles the docs.

@jposada202020 jposada202020 requested review from a team and removed request for a team March 20, 2023 11:46
@BiffoBear
Copy link
Author

Used strings for the typing of RGBLED.init(). Reverted the Sphinx conf.py, requirements.txt and optional_requirements.txt. Now passes CI

@jposada202020 jposada202020 requested a review from a team March 21, 2023 10:52
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.

Added feedback below, thanks for the submission!

adafruit_rgbled.py Outdated Show resolved Hide resolved
adafruit_rgbled.py Outdated Show resolved Hide resolved
adafruit_rgbled.py Outdated Show resolved Hide resolved
def __init__(self, red_pin, green_pin, blue_pin, invert_pwm=False):
def __init__(
self,
red_pin: Union["microcontroller.Pin", PWMOut, "PWMChannel"],
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason you're using quotes instead of importing within the try/except block?

Copy link
Author

Choose a reason for hiding this comment

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

For the "microcontroller.Pin" I was getting "Not Implemented" exceptions in my PyCharm environment. That now happens with the from pwmio import PWMOut which makes this moot. I've changed this to use from microcontroller import Pin

For the "PWMChannel" I couldn't find a library with PWMChannel that I can import on my laptop. The #circuitpython-dev Discord channel suggested using quotes. If you know of a library that I can use, I'm happy to make that change too.

Copy link
Author

@BiffoBear BiffoBear Apr 12, 2023

Choose a reason for hiding this comment

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

…and the CI is failing with

  File "/home/runner/work/Adafruit_CircuitPython_RGBLED/Adafruit_CircuitPython_RGBLED/adafruit_rgbled.py", line 98, in RGBLED
    red_pin: Union[microcontroller.Pin, PWMOut, "PWMChannel"],
                   ^^^^^^^^^^^^^^^
NameError: name 'microcontroller' is not defined

Error: Process completed with exit code 2.

Which may be why I used the quotes. Can't remember TBH!

adafruit-blinka is in requirements.txt and that's where I thought the microcontroller module lives.
I'll push a commit with quotes to have something that passes the CI.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like it's failing because it isn't actually imported in the file. If you add it as an import to the try/except block, it should work fine without quotes. Same thing for PWMChannel.

Copy link
Author

Choose a reason for hiding this comment

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

With import statements in the try/except block I can get Pin to pass CI but not PWMChannel.

try:
    from typing import Union, Optional, Type
    from types import TracebackType
    from microcontroller import Pin
    from adafruit_pca9685 import PWMChannel
    from circuitpython_typing.led import ColorBasedColorUnion
except ImportError:
    pass
...
def __init__(
    self,
    red_pin: Union[Pin, PWMOut, PWMChannel],
    green_pin: Union[Pin, PWMOut, PWMChannel],
    blue_pin: Union[Pin, PWMOut, PWMChannel],
    invert_pwm: bool = False,
) -> None:

I've added adafruit-pca9685 to requirements.txt but this did not help.

Warning, treated as error:
autodoc: failed to import module 'adafruit_rgbled'; the following exception was raised:
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.11.3/x64/lib/python3.11/site-packages/sphinx/ext/autodoc/importer.py", line 60, in import_module
    return importlib.import_module(modname)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.11.3/x64/lib/python3.11/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1206, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1178, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1149, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 690, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 940, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/home/runner/work/Adafruit_CircuitPython_RGBLED/Adafruit_CircuitPython_RGBLED/adafruit_rgbled.py", line 36, in <module>
    class RGBLED:
  File "/home/runner/work/Adafruit_CircuitPython_RGBLED/Adafruit_CircuitPython_RGBLED/adafruit_rgbled.py", line 100, in RGBLED
    red_pin: Union[Pin, PWMOut, PWMChannel],
                                ^^^^^^^^^^
NameError: name 'PWMChannel' is not defined

Error: Process completed with exit code 2.```

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.

Thanks for the changes! I've added followup to the points above.

@BiffoBear
Copy link
Author

Thanks for the changes! I've added followup to the points above.

Your welcome, thanks for taking the time to review.
I've done my best, but I still can't get PWMChannel past CI!

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

I took a look at the actions failure and found that it was just missing the module that PWMChannel is imported from. The pip name was slightly different from what was in the requirements.txt file.

I pushed a new commit that changes and now the docs build is suceeding.

These changes look good to me. Thanks for working on this @BiffoBear!

@FoamyGuy FoamyGuy merged commit 6fb99e4 into adafruit:main Jul 31, 2023
1 check passed
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Aug 1, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_MCP9808 to 3.3.21 from 3.3.20:
  > Merge pull request adafruit/Adafruit_CircuitPython_MCP9808#37 from jposada202020/updating_example

Updating https://github.com/adafruit/Adafruit_CircuitPython_AdafruitIO to 5.7.2 from 5.7.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_AdafruitIO#98 from ForgottenProgramme/mahe-typehint

Updating https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer to 4.2.0 from 4.1.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#63 from michalpokusa/external-routes-websockets-sse

Updating https://github.com/adafruit/Adafruit_CircuitPython_NeoKey to 1.1.0 from 1.0.12:
  > Merge pull request adafruit/Adafruit_CircuitPython_NeoKey#11 from EAGrahamJr/all-the-buttons

Updating https://github.com/adafruit/Adafruit_CircuitPython_RGBLED to 1.1.17 from 1.1.16:
  > Merge pull request adafruit/Adafruit_CircuitPython_RGBLED#23 from BiffoBear/Add_typing

Updating https://github.com/adafruit/Adafruit_CircuitPython_TinyLoRa to 2.2.16 from 2.2.15:
  > Merge pull request adafruit/Adafruit_CircuitPython_TinyLoRa#50 from samatjain/type-annotations

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
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.

Missing Type Annotations
3 participants