-
Notifications
You must be signed in to change notification settings - Fork 58
re-export symbols (classes, constants) to accessible & ergonomic locations #101
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
Conversation
Hmm.. seems like pylint doesn't recognise redundant aliases. Ruff, pyflake, etc. recommend redundant aliases for re-exports. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pantheraleo-7 Thanks for submitting this. Sorry it took so long to get a review / response.
I have a few other comments with suggested changes.
I also think this same PR should go ahead and remove the original P0
etc... duplicated constants and update the examples and readme.
8679931
to
0b7816c
Compare
4d17126
to
0b7816c
Compare
# define Pin to avoid the error: | ||
# def read(self, pin: Pin, is_differential: bool = False) -> int: | ||
# NameError: name 'Pin' is not defined | ||
Pin = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this pattern in many libraries which is flat out wrong and results in more type checking errors. The correct solution is forward references, see ads1x15.py
diff#130/139.
Also, ideally, the imports should be guarded by a if typing.TYPE_CHECKING
block but it isn't strictly necessary since the imports are already in a try-except block but the former is more pythonic and correct.
So the full correct pattern to use here is:
try:
from typing import TYPE_CHECKING # and other `typing` imports
if TYPE_CHECKING:
# imports from modules other than `typing` will come here
except ImportError:
pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this some more. One more question, and request to update the docstrings to match new type annotations.
ads modules to the generic adsx module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
I tested the latest version using simpletest script and a potentiometer connected to A0.
This will need some guide updates when merged. At least on this page: https://learn.adafruit.com/adafruit-4-channel-adc-breakouts/python-circuitpython perhaps others since this library supports multiple different breakouts. I'll do a more thorough sweep and update all required pages when I merge this.
The learn page linked above is updated now to reflect the changes from this PR. I searched learn repo and site for any other usages of ADS1x15 library and did not find any. |
Updating https://github.com/adafruit/Adafruit_CircuitPython_ADS1x15 to 3.0.0 from 2.4.4: > Merge pull request adafruit/Adafruit_CircuitPython_ADS1x15#101 from pantheraleo-7/main
Addresses #100
If the PR gets accepted, I'll be willing to update examples, docs, etc.
Per this PR, the usage example would like this:
I'd say this is more pythonic and readable.
This change is completely backwards compatible.