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

Fix _bleio doc comments #9540

Merged
merged 2 commits into from
Aug 22, 2024
Merged

Fix _bleio doc comments #9540

merged 2 commits into from
Aug 22, 2024

Conversation

elpekenin
Copy link

Draft because I will be fixing anything else I find while i working on Adafruit_CircuitPython_BLE's type hints

I just found out this because mypy complained that add_to_service was missing a positional arg (self and service were expected based on comment, but it really is a classmethod and only service has to be passed in)

This also lead me to seeing __init__ is not defined... Does that mean that trying to run Characteristic() will raise? If so, should we perhaps change the type hint from -> None to -> NoReturn or the like?

@elpekenin
Copy link
Author

I have almost finished fixing the typing of BLE lib and didnt find any more issues with respect to _bleio stubs, ready for review here (:

@elpekenin elpekenin marked this pull request as ready for review August 22, 2024 05:55
Copy link
Collaborator

@dhalbert dhalbert 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 workign on this.

This also lead me to seeing __init__ is not defined... Does that mean that trying to run Characteristic() will raise? If so, should we perhaps change the type hint from -> None to -> NoReturn or the like?

Yes, calling the constructor raises an exception:

>>> from _bleio import Characteristic
>>> c = Characteristic()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot create 'Characteristic' instances

The __init__() signature is not shown directly in the documentation, but the .pyi stubs file has it. See the documentation here:
https://docs.circuitpython.org/en/latest/shared-bindings/_bleio/index.html#bleio.Characteristic.

I don't remember why we put that comment in the __init()__ docstring, when there is no init. Locally I changed the doc to remove the __init()__ signature and put its comment into the class comment, and that seems to work fine: the doc and the stubs build without errors.

//| class Characteristic:
//|     """Stores information about a BLE service characteristic and allows reading
//|     and writing of the characteristic's value.
//|
//|     There is no regular constructor for a `Characteristic`. A new local `Characteristic` can be created
//|     and attached to a Service by calling `add_to_service()`.
//|     Remote Characteristic objects are created by `Connection.discover_remote_services()`
//|     as part of remote Services.
//|     """
//|

//|     def add_to_service(

Note the extra blank line after the closing """, which was added by the pre-commit check.

Also note I put backticks around "Characteristic" in the comment.

Removing the __init()__ signature should also be done for _bleio.Descriptor and other classes that don't have a public constructor.

@elpekenin
Copy link
Author

My concern was not really showing vs not showing the __init__ signature on the docs/stubs, but being able to type-hint it correctly.

Skipping it on the doc-comment, {MyPy/Intellisense/whatever} would assume we have a "default" def __init__(self) -> None signature, which is valid to call, while having it marked as -> NoReturn would explicitly mark it is a forbidden operation....

However, i've been testing locally for a bit and can't find a way to "describe" this behavior such that MyPy can give useful feedback in this aspect. In fact, doing it a NoReturn/Never got MyPy to mark valid things as invalid.

I think the best idea is to keep the __init__ in C-comments and the stubs to display the "this doesnt exist" message.

@dhalbert
Copy link
Collaborator

OK, I see what you mean. In pure Python, I think you always have an __init__(), explicit or implicit, that is callable. We are circumventing that because the class implemented natively and we can prevent that.

This is interesting: https://stackoverflow.com/questions/46779046/correct-type-annotation-for-init. It says that the return type annotation could just be omitted.

@elpekenin elpekenin changed the title [WIP] Fix _bleio doc comments Fix _bleio doc comments Aug 22, 2024
@elpekenin
Copy link
Author

Im not sure about this, but i think that adding -> None is required for type checkers to "look into" your function's body. Without the explicit typing (and given the lack of typed arguments), tooling will/could say "oh, this function is yet to be typed, im not running any analysis on its body"; so, even if if is not required, i think it is nice to add.

The idea for NoReturn (or Never) is to represent a function that never has an actual return endless loop, sys.exit killing the program, or -like this scenario- a function that always raises). But anyway, as stated above, i cant get MyPy to understand what i mean (or rather, im unable to represent the concept), so i'm keeping it as is.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

All set after some side discussion on this.

@dhalbert dhalbert merged commit 88e8a3c into adafruit:main Aug 22, 2024
342 checks passed
@elpekenin elpekenin deleted the fix/_bleio-stubs branch August 22, 2024 14:32
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.

2 participants