- Notifications
You must be signed in to change notification settings - Fork673
feat(api): Use protocols and overload to type hint mixins#3080
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
feat(api): Use protocols and overload to type hint mixins#3080
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Currently mixins like ListMixin are type hinted to return baseRESTObject instead of a specific class like `MergeRequest`.The GetMixin and GetWithoutIdMixin solve this problem by defininga new `get` method for every defined class. However, this createsa lot of duplicated code.`typing.Protocol` can be used to type hint that the mixed in methodwill return a class matching attribute `_obj_cls`. The type checkerwill lookup the mixed in class attribute and adjust the return typeaccordinly.Delete `tests/unit/meta/test_ensure_type_hints.py` file as the `get`method is no required to be defined for every class.Signed-off-by: Igor Ponomarev <igor.ponomarev@collabora.com>
igorp-collabora commentedJan 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This turned out to be a larger commit than I expected. I can probably split it by a mixin. For example, first commit for Get and GetWithoutId mixins, second one for ListMixin... Also this is a pretty ad-hoc solution but requires the least amount of changes. Ideally making a I made a table of the attributes each mixin uses:#2228 (comment) Looking at it the RESTManager hierarchy would probably be a base object without |
| @@ -347,7 +347,6 @@ class RESTManager: | |||
| _create_attrs: g_types.RequiredOptional = g_types.RequiredOptional() | |||
| _update_attrs: g_types.RequiredOptional = g_types.RequiredOptional() | |||
| _path: Optional[str] = None | |||
| _obj_cls: Optional[Type[RESTObject]] = None | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The_obj_cls needs to be specific for each class. Having a_obj_cls as None in base class will prevent Protocol from working.
| @@ -397,10 +397,11 @@ def auth(self) -> None: | |||
| The `user` attribute will hold a `gitlab.objects.CurrentUser` object on | |||
| success. | |||
| """ | |||
| self.user = self._objects.CurrentUserManager(self).get() | |||
| user = self._objects.CurrentUserManager(self).get() | |||
| self.user = user | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For some reasonself.user will now be type evaluated asOptional[User]. I had to separate user to a new variable that can't be None.
| *, | ||
| iterator: Literal[True] = True, | ||
| **kwargs: Any, | ||
| ) -> base.RESTObjectList: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added an overload to the list method so that passingiterator will change if a list is returned or an iterator. This can be separated to another commit and MR.
AlsoRESTObjectList is not generic. I want to create a follow-up MR there it will be made generic to iterate over a specific type.
| @@ -104,12 +104,12 @@ def reset_gitlab(gl: gitlab.Gitlab) -> None: | |||
| ) | |||
| continue | |||
| fordeploy_token in group.deploytokens.list(): | |||
| forgroup_deploy_token in group.deploytokens.list(): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Becauselist() now returns a list containing a specific type it would case a type error here because the same variable is used between two different types.
| @@ -4,7 +4,9 @@ | |||
| https://docs.gitlab.com/ee/api/projects.html#list-projects-starred-by-a-user | |||
| """ | |||
| from typing import Any, cast, Dict, List, Optional, Union | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For some reason without delayed annotations type checks failed to resolve types forUserProject.list. This means the quotes around the type annotation had to be removed or managers would not be initialized.
@igorp-collabora Thanks for this. Not ignoring your PRs. Just I haven't had time yet to fully review them. Hopefully this week or next week. Sorry for the delay. Thanks for the PRs! They look interesting from the description 👍 |
codecovbot commentedJan 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #3080 +/- ##==========================================+ Coverage 96.68% 97.19% +0.50%========================================== Files 96 96 Lines 6213 5913 -300 ==========================================- Hits 6007 5747 -260+ Misses 206 166 -40
Flags with carried forward coverage won't be shown.Click here to find out more.
|
No worries. I want to experiment with reworking RESTManager hierarchy meanwhile. |
The#3083 is a cleaner solution but more verbose. This one builds more work-around code on top of existing one. |
Changes
Currently mixins like ListMixin are type hinted to return base RESTObject instead of a specific class like
MergeRequest.The GetMixin and GetWithoutIdMixin solve this problem by defining a new
getmethod for every defined class. However, this creates a lot of duplicated code.typing.Protocolcan be used to type hint that the mixed in method will return a class matching attribute_obj_cls. The type checker will lookup the mixed in class attribute and adjust the return type accordinly.Delete
tests/unit/meta/test_ensure_type_hints.pyfile as thegetmethod is no required to be defined for every class.Closes#2228
Closes#2062