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

Remove # type: ignore[call-arg] in HCI_Command builders #360

Merged
merged 1 commit into from
Dec 3, 2023

Conversation

zxzxwu
Copy link
Collaborator

@zxzxwu zxzxwu commented Dec 2, 2023

No description provided.

@zxzxwu zxzxwu changed the title Remove # type: ignore[call-arg] by allowing Remove # type: ignore[call-arg] in HCI_Command builders Dec 2, 2023
@@ -2103,7 +2104,11 @@ def parse_return_parameters(cls, parameters):
return_parameters.fields = cls.return_parameters_fields
return return_parameters

def __init__(self, op_code, parameters=None, **kwargs):
def __init__(self, op_code=-1, parameters=None, **kwargs):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another solution is in the @HCI_Command.command decorator, converting classes to dataclass so that a better static check can be provided.

Copy link
Collaborator Author

@zxzxwu zxzxwu Dec 2, 2023

Choose a reason for hiding this comment

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

Oh we can't until 3.11. Type checker cannot identify the type context provided in decorator, unless we have @typing.dataclass_transform. https://docs.python.org/3/library/typing.html#typing.dataclass_transform

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like 3.11 will make things easier in that regard. I remember that @uael looked into a mypy plugin for this a while back, but that ends up being a lot of work. I wonder if there'd be a way to create a forward-compatibility way of using the 3.11 @typing.dataclass_transform on the code, but just "fake" it for versions before 3.11. We do want to keep support for older pythons for a while (not sure how long yet, we also do want to evolve), but since most users will be on newer versions, it would be nice to take advantage of some of the new features.

Copy link
Collaborator Author

@zxzxwu zxzxwu Dec 3, 2023

Choose a reason for hiding this comment

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

I did some experiments and I found it's not that simple. PEP 681 only provides a way that dataclass doesn't need to be added directly. For example, it can synthesize __init__() with parameters for HCI_LE_Setup_ISO_Data_Path_Command because I add fields statically, but other classes only having decorators cannot be identified. According to python/mypy#6063 (comment), static type checker is probably never able to check such a use case.

@@ -2103,7 +2104,11 @@ def parse_return_parameters(cls, parameters):
return_parameters.fields = cls.return_parameters_fields
return return_parameters

def __init__(self, op_code, parameters=None, **kwargs):
def __init__(self, op_code=-1, parameters=None, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like 3.11 will make things easier in that regard. I remember that @uael looked into a mypy plugin for this a while back, but that ends up being a lot of work. I wonder if there'd be a way to create a forward-compatibility way of using the 3.11 @typing.dataclass_transform on the code, but just "fake" it for versions before 3.11. We do want to keep support for older pythons for a while (not sure how long yet, we also do want to evolve), but since most users will be on newer versions, it would be nice to take advantage of some of the new features.

@zxzxwu zxzxwu merged commit 3adcc8b into google:main Dec 3, 2023
51 checks passed
@zxzxwu zxzxwu deleted the hci branch December 12, 2023 05:53
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