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

Added type annotations #17

Merged
merged 6 commits into from
Apr 27, 2023
Merged

Added type annotations #17

merged 6 commits into from
Apr 27, 2023

Conversation

patenaud
Copy link
Contributor

No description provided.

@patenaud patenaud changed the title Added type annotations -> Missing Type Annotations #12 Added type annotations Apr 26, 2023
@FoamyGuy
Copy link
Contributor

@patenaud Thanks for working on this!

I'll answer some questions you posted over in the issue here.

the microcontroller.Pin and busio.I2C which are passed as arguments in the OV7670 init were not imported. I added the 2 import statements to quiet the complaint but I'm not sure if this was OK. Please confirm.

adding the new imports for them is the proper thing to do. Those new imports should get put inside of the try block with the typing import as the first one i.e.:

try:
    from typing import List, Optional, Union
    from busio import I2C
    from microcontroller import Pin
except ImportError:
    pass

For the pylint complaints about too many arguments you can supress it with a pylint disable line like this:

# pylint: disable=too-many-arguments

If you add that to the functions it's warning about it will opt them out of the test and continue allowing it to pass.

In this case that's fine because your PR didn't add or change the number of arguments anyway, I'm surprised it hadn't been failing actions previously.

@patenaud
Copy link
Contributor Author

patenaud commented Apr 26, 2023

You're welcome! It's been very educational. :-)
I realized that the pylint comment was there but it was after the arguments. I moved it to before the arguments and that seems to have fixed the issue.

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've had a look over this and found a few changes I think would be good.

adafruit_ov7670.py Outdated Show resolved Hide resolved
adafruit_ov7670.py Outdated Show resolved Hide resolved
adafruit_ov7670.py Outdated Show resolved Hide resolved
adafruit_ov7670.py Outdated Show resolved Hide resolved
adafruit_ov7670.py Outdated Show resolved Hide resolved
@patenaud
Copy link
Contributor Author

@FoamyGuy I really appreciate the feedback. I'm sure it took you more time to help me than to just do it.

@tekktrik tekktrik linked an issue Apr 27, 2023 that may be closed by this pull request
12 tasks
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.

Looks good to me. Thanks again @patenaud!

I'm always happy to spend time helping out other folks who are interested in contributing! 😄

@FoamyGuy FoamyGuy merged commit 29536e6 into adafruit:main Apr 27, 2023
1 check passed
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
2 participants