Skip to content

adding type information #27

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 3 commits into from
Apr 16, 2022
Merged

adding type information #27

merged 3 commits into from
Apr 16, 2022

Conversation

FoamyGuy
Copy link
Contributor

resolves #21.

The main thing I am somewhat uncertain of is the * arg in the constructor here:
https://github.com/FoamyGuy/Adafruit_CircuitPython_VC0706/blob/d56a319b59a5f211617c90bb9963d03b405cc879/adafruit_vc0706.py#L93

I tried Any type but that did not seem to work. I'm not sure what the proper way to type this wildcard argument is. Perhaps leaving it untyped is standard practice? If anyone knows about this, or knows of an example where this is done in another library let me know.

@tekktrik
Copy link
Member

* signifies that any arguments after it are keyword only! I think in this case it doesn't have a type.

Here's the relevant PEP, for reference: https://peps.python.org/pep-3102/#:~:text=Keyword%2Donly%20arguments%20are%20not,therefore%20'required%20keyword'%20arguments.

@tekktrik
Copy link
Member

tekktrik commented Apr 14, 2022

It sounds like if it were *ignore or something else it could be typed as List[Any] but the star shortcut itself doesn't have a type?

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.

Nice! A few questions and suggestions, but otherwise looks good!

@FoamyGuy
Copy link
Contributor Author

It sounds like if it were *ignore or something else it could be typed as List[Any] but the star shortcut itself doesn't have a type?

I'm not sure if it does. It would seem tricky though because it can represent multiple different arguments, so no single type could really describe it in all cases.

@tekktrik
Copy link
Member

I'm not sure if it does. It would seem tricky though because it can represent multiple different arguments, so no single type could really describe it in all cases.

I think using only * means the program should raise an error if additional positional arguments are found, so theoretically, it can't be ANY type, since it being anything will raise an error, unless CircuitPython implementation deviates from the PEP. It would be like typing the / character that can be used to signify positional-only arguments (which is less commonly used I think but still valid), in that it seems more to be more of a sentinel character than an actual argument itself.

@FoamyGuy
Copy link
Contributor Author

Interesting, I never knew about that usage of the / character. I would assume CircuitPython does follow the PEP as closely as possible because I know that is one of the primary goals is to keep compatibility with CPython whenever possible. Though I'm not 100% certain of the behavior in CircuitPython (or really CPython for that matter).

Thank you for looking this over and for explanation about the different Buffer protocols. I think I'm starting to get a better understanding of when it's appropriate to use which one.

I think I've got all of the requested changes in the latest commit. If you have another chance to take a look @tekktrik let me know if anything else is needed.

@tekktrik
Copy link
Member

My pleasure! I think that's everything!

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.

Looks good to me!

@tekktrik tekktrik merged commit cf51ea2 into adafruit:main Apr 16, 2022
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Apr 16, 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
2 participants