Skip to content

Adding typing #86

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 10 commits into from
Sep 7, 2022
Merged

Adding typing #86

merged 10 commits into from
Sep 7, 2022

Conversation

scirelli
Copy link
Contributor

@scirelli scirelli commented Aug 31, 2022

Adding .venv to the ignore list so it doesn't accidentally get added
Adding py.typed file to signify this library has typing information
Fixed typing errors found by mypy

Addresses issue #83

@scirelli
Copy link
Contributor Author

I'm not sure how to test the changes. Any advice?

@scirelli
Copy link
Contributor Author

Looks like black made some changes. Is there a way to rerun the pipeline?

@tekktrik tekktrik linked an issue Sep 1, 2022 that may be closed by this pull request
@tekktrik tekktrik requested a review from a team September 1, 2022 14:05
@tekktrik
Copy link
Member

tekktrik commented Sep 1, 2022

Nice! I'll try to take a look at this soon, but maybe someone else will first!

Copy link

@tammymakesthings tammymakesthings left a comment

Choose a reason for hiding this comment

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

These changes all look reasonable to me and I don’t see any problems. But I can’t find this board in my collection, so I personally would feel better if someone verified it on the hardware before merging.

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.

Feedback below! Also, would you mind adding type annotations for the properties getter and setters as well? Thanks!

@scirelli
Copy link
Contributor Author

scirelli commented Sep 2, 2022

These changes all look reasonable to me and I don’t see any problems. But I can’t find this board in my collection, so I personally would feel better if someone verified it on the hardware before merging.

I have a bbb and ads1115, but not an ads1015

@tekktrik
Copy link
Member

tekktrik commented Sep 2, 2022

@scirelli, can you use the generics from the typing module like Dict and List instead of built-ins? Using built-ins as generics wasn't available in Python 3.7.

@tekktrik
Copy link
Member

tekktrik commented Sep 6, 2022

@scirelli let me know when I should re-review

@scirelli
Copy link
Contributor Author

scirelli commented Sep 6, 2022

I have not had a chance to test on actual hardware. Other than that I addressed all PR comments.

Which tests should I run? I was looking at the simple one in examples.

@tekktrik
Copy link
Member

tekktrik commented Sep 6, 2022

I also realize that I haven't reviewed the property getters and setters, I'll do that in a few hours. Sorry for the delay!

@tekktrik tekktrik self-requested a review September 6, 2022 22:33
@scirelli scirelli requested a review from tekktrik September 7, 2022 14:47
@scirelli scirelli requested a review from tekktrik September 7, 2022 19:40
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.

Great work!

@scirelli
Copy link
Contributor Author

scirelli commented Sep 7, 2022

Thanks! I learned a few things, still new to Python typing.

@tekktrik tekktrik merged commit fe1337c into adafruit:main Sep 7, 2022
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Sep 8, 2022
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