Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore: improve type-hinting for managers#1512

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

Merged
nejch merged 3 commits intopython-gitlab:masterfromJohnVillalovos:jlvillal/type_managers
Sep 8, 2021
Merged

chore: improve type-hinting for managers#1512

nejch merged 3 commits intopython-gitlab:masterfromJohnVillalovos:jlvillal/type_managers
Sep 8, 2021

Conversation

JohnVillalovos
Copy link
Member

@JohnVillalovosJohnVillalovos commentedJun 12, 2021
edited
Loading

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.

@codecov-commenter
Copy link

codecov-commenter commentedJun 12, 2021
edited
Loading

Codecov Report

Merging#1512 (c8611e0) intomaster (330d69c) willincrease coverage by0.22%.
The diff coverage is98.66%.

@@            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
FlagCoverage Δ
cli_func_v481.28% <98.66%> (+0.50%)⬆️
py_func_v480.56% <98.66%> (+0.48%)⬆️
unit82.80% <98.66%> (+0.46%)⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

Impacted FilesCoverage Δ
gitlab/v4/cli.py81.20% <0.00%> (-0.31%)⬇️
gitlab/base.py89.69% <100.00%> (+0.25%)⬆️
gitlab/v4/objects/boards.py100.00% <100.00%> (ø)
gitlab/v4/objects/commits.py94.36% <100.00%> (+0.16%)⬆️
gitlab/v4/objects/container_registry.py83.33% <100.00%> (ø)
gitlab/v4/objects/deployments.py100.00% <100.00%> (ø)
gitlab/v4/objects/discussions.py100.00% <100.00%> (ø)
gitlab/v4/objects/epics.py74.35% <100.00%> (+0.67%)⬆️
gitlab/v4/objects/groups.py87.20% <100.00%> (+3.36%)⬆️
gitlab/v4/objects/issues.py88.00% <100.00%> (+1.04%)⬆️
... and10 more

@JohnVillalovos
Copy link
MemberAuthor

So in my quick check I could just use the annotations to create the managers. As all the annotations which are of type*Manager are also in the_managers attribute.

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?

@JohnVillalovosJohnVillalovos self-assigned thisJun 13, 2021
Copy link
Member

@nejchnejch left a comment

Choose a reason for hiding this comment

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

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 CLIhttps://github.com/python-gitlab/python-gitlab/blob/master/gitlab/cli.py#L92-L96

# namespace = gitlab.v4.objectsmanagers= [manformaninnamespace.__dict__ifman.endswith("Manager")]ifmanagernotinmanagers:continue...

(something prettier than this I guess, but you get the idea :) )

When I look at this in your PR:

classProjectDeployment(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?

@nejchnejch requested a review frommax-wittigJune 13, 2021 18:48
@JohnVillalovos
Copy link
MemberAuthor

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).

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.

Copy link
Member

@nejchnejch left a comment

Choose a reason for hiding this comment

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

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).

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:

mgr_cls_name=cls.__name__+"Manager"
mgr_cls=getattr(gitlab.v4.objects,mgr_cls_name)

forclsingitlab.v4.objects.__dict__.values():
ifnotisinstance(cls,type):
continue
ifissubclass(cls,gitlab.base.RESTManager):
ifcls._obj_clsisnotNone:
classes.append(cls._obj_cls)
classes.sort(key=operator.attrgetter("__name__"))

]=getattr(gitlab.v4.objects,self.cls.__name__+"Manager")

... 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 thatwe don't have any type-hints for them and so editors which understandtype-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
@JohnVillalovos
Copy link
MemberAuthor

@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 simplydo:      class ExampleClass(CRUDMixin, RESTObject):          my_manager: MyManagerAny type-annotation that annotates it to be of type *Manager (with theexception of RESTManager) will cause the manager to be created on theobject.
@JohnVillalovosJohnVillalovos marked this pull request as draftSeptember 8, 2021 15:24
Add an additional check to attempt to solve the flakiness of thetest_merge_request_should_remove_source_branch() test.
@nejch
Copy link
Member

@nejch Is there anything else that needs to be done for this to get merged?

I think it'll also be ready@JohnVillalovos just ping me when you're finished with the current rebase

@JohnVillalovosJohnVillalovos marked this pull request as ready for reviewSeptember 8, 2021 16:29
@JohnVillalovos
Copy link
MemberAuthor

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.

@nejchnejch merged commit5247e8b intopython-gitlab:masterSep 8, 2021
@JohnVillalovosJohnVillalovos deleted the jlvillal/type_managers branchJanuary 4, 2022 06:55
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@max-wittigmax-wittigmax-wittig left review comments

@nejchnejchnejch approved these changes

Assignees

@JohnVillalovosJohnVillalovos

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@JohnVillalovos@codecov-commenter@nejch@max-wittig

[8]ページ先頭

©2009-2025 Movatter.jp