- Notifications
You must be signed in to change notification settings - Fork673
chore: add type-hints to gitlab/v4/objects/users.py#1515
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
chore: add type-hints to gitlab/v4/objects/users.py#1515
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-commenter commentedJun 13, 2021 • 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 Report
@@ Coverage Diff @@## master #1515 +/- ##======================================= Coverage 91.15% 91.16% ======================================= Files 74 74 Lines 4172 4176 +4 =======================================+ Hits 3803 3807 +4 Misses 369 369
Flags with carried forward coverage won't be shown.Click here to find out more.
|
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 just have a question about how this will go if we continue this way for all modules. I haven't looked at alternatives at all so just asking atm.
| def get(self, id: Union[str, int], lazy: bool = False, **kwargs: Any) -> User: | ||
| return cast(User, super().get(id=id, lazy=lazy, **kwargs)) |
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.
question: I already merged this for projects but now it has me thinking, is there any way to avoid adding all this boilerplate for typing? There will be alot of these I imagine if as we go module by module, and I guess will need to be copy/pasted for a lot of mixin usage.
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 have tried and tried again and tried again... But so far I can't figure out a way to do it. If you can figure it out it will be awesome!
There are ways to do it if you return aself object. But since we are returningself._obj_cls() it doesn't seem like things are smart enough to understand that yet. Well at least notmypy
This will help editors and the type-checker in knowing thatgl.users.get() returns aUser object. Otherwise it will think it returns aRESTObject which isn't as useful.
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.
Oh, you will probably like my idea for handlinggl.users.list() even less 😜
At the moment my best solution for that seems to be adding aUserList class or something like that. I haven't figured it out yet but thinking about it.
Adding type-hints to gitlab/v4/objects/users.py
@nejch Is there anything else that needs to be done for this to get merged? |
Sorry@JohnVillalovos I never ended up diving into whether we could avoid the extra code in the discussion above. Maybe I could try to find any typed projects that might rely heavily on managers and mixins? |
Works for me. Would be cool if cleaner solution. So far I haven't been able to figure one out 😟 |
@nejch Curious if you had time to think about this. My thought is that this works but isn't pretty. But sometimes that is what we have to do. And it is only two lines of code. As the expression says, "Perfect is the enemy of good" 😊 |
@JohnVillalovos I also haven't done much in this direction so let's merge :D Yeah, it won't be much to change if we do figure it out. |
Adding type-hints to gitlab/v4/objects/users.py