chore: improve type-hinting for managers #1512
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1512 +/- ##
==========================================
+ Coverage 91.15% 91.37% +0.22%
==========================================
Files 74 74
Lines 4172 4291 +119
==========================================
+ Hits 3803 3921 +118
- Misses 369 370 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
So in my quick check I could just use the annotations to create the managers. As all the annotations which are of type But I think to be safe would probably be better to do this as a first step and then later switch to using annotations only. Any thoughts? |
There was a problem hiding this comment.
I would personally prefer if we could define things in one place and avoid adding too much duplication, and we would also not need to create custom tests to enforce it (and just test _create_managers instead).
Using __annotations__ for this would be convenient although a little magic. But the syntax is so clean it might be less cryptic to new contributors who don't care how managers are created. It could be combined with a simple check that it's a real manager, similar to what we already do for the CLI https://github.com/python-gitlab/python-gitlab/blob/master/gitlab/cli.py#L92-L96
# namespace = gitlab.v4.objects
managers = [man for man in namespace.__dict__ if man.endswith("Manager")]
if manager not in managers:
continue
...(something prettier than this I guess, but you get the idea :) )
When I look at this in your PR:
class ProjectDeployment(SaveMixin, RESTObject):
mergerequests: ProjectDeploymentMergeRequestManager
_managers = (("mergerequests", "ProjectDeploymentMergeRequestManager"),)The new syntax is just so nice and reminds me of dataclasses, and the old one starts to look ugly and I'd just remove it 😸 On the other hand I don't want to force something that's too magic, @max-wittig what's your opinion here?
Agreed. I am going to update the PR to just use annotations. We can then decide which one we like better. I do like the annotation only one. |
There was a problem hiding this comment.
I would personally prefer if we could define things in one place and avoid adding too much duplication, and we would also not need to create custom tests to enforce it (and just test
_create_managersinstead).Agreed.
I am going to update the PR to just use annotations. We can then decide which one we like better.
I do like the annotation only one.
I kind of really like this as well, despite the magic so I'll approve it (just needs a rebase and I just have a style question) 😀 When you think about it, the dynamic manager creation has always been a bit magic and considering the stuff we already do in:
python-gitlab/gitlab/v4/cli.py
Lines 177 to 178 in 6abf13a
python-gitlab/gitlab/v4/cli.py
Lines 315 to 321 in 6abf13a
python-gitlab/gitlab/v4/cli.py
Line 49 in 6abf13a
... this is not really outrageous compared to that :) But would like also @max-wittig to take a quick look.
The 'managers' are dynamically created. This unfortunately means that we don't have any type-hints for them and so editors which understand type-hints won't know that they are valid attributes. * Add the type-hints for the managers we define. * Add a unit test that makes sure that the type-hints and the '_managers' attribute are kept in sync with each other. * Add unit test that makes sure specified managers in '_managers' have a name ending in 'Managers' to keep with current convention. * Make RESTObject._managers always present with a default value of None. * Fix a type-issue revealed now that mypy knows what the type is
|
@nejch Is there anything else that needs to be done for this to get merged? |
Convert our manager usage to be done via type annotations.
Now to define a manager to be used in a RESTObject subclass can simply
do:
class ExampleClass(CRUDMixin, RESTObject):
my_manager: MyManager
Any type-annotation that annotates it to be of type *Manager (with the
exception of RESTManager) will cause the manager to be created on the
object.
Add an additional check to attempt to solve the flakiness of the test_merge_request_should_remove_source_branch() test.
I think it'll also be ready @JohnVillalovos just ping me when you're finished with the current rebase |
@nejch It is ready for review. I added a fix for flakiness in one of the functional tests. |
The 'managers' are dynamically created. This unfortunately means that
we don't have any type-hints for them and so editors which understand
type-hints won't know that they are valid attributes.
'_managers' attribute are kept in sync with each other.
have a name ending in 'Managers' to keep with current convention.
None.