NotificationsYou must be signed in to change notification settings
Fork675
Star2.4k

fix: add a check to ensure the MRO is correct#1352

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

Jump to bottom

Merged

nejch merged 2 commits intopython-gitlab:masterfromJohnVillalovos:jlvillal/fix_mro

Apr 27, 2021

Merged

fix: add a check to ensure the MRO is correct#1352

nejch merged 2 commits intopython-gitlab:masterfromJohnVillalovos:jlvillal/fix_mro

Apr 27, 2021

Conversation

Copy link

Member

JohnVillalovos commentedMar 1, 2021•
edited
Loading

This should be merged after:
#1344 DONE

Add a check to ensure the MRO (Method Resolution Order) is correct for classes in
gitlab.v4.objects.

An example of an incorrect definition:    class ProjectPipeline(RESTObject, RefreshMixin, ObjectDeleteMixin):                          ^^^^^^^^^^ This should be at the end.Correct way would be:    class ProjectPipeline(RefreshMixin, ObjectDeleteMixin, RESTObject):                                      Correctly at the end ^^^^^^^^^^

Also fix classes which have the issue.

Copy link

Member

max-wittig commentedMar 2, 2021

Thanks for the MR. Could you elaborate why this matters? I know what you can override methods based on the order, but other than that, I'm unsure. Would be interesting to know@JohnVillalovos

Copy link

MemberAuthor

JohnVillalovos commentedMar 2, 2021

Thanks for the MR. Could you elaborate why this matters? I know what you can override methods based on the order, but other than that, I'm unsure. Would be interesting to know@JohnVillalovos

Sure. Let me write up a better explanation and post it. It will have to be after my work-day is over here in Oregon.

Copy link

MemberAuthor

JohnVillalovos commentedMar 3, 2021

Thanks for the MR. Could you elaborate why this matters? I know what you can override methods based on the order, but other than that, I'm unsure. Would be interesting to know@JohnVillalovos
Sure. Let me write up a better explanation and post it. It will have to be after my work-day is over here in Oregon.

I have put comments in the test file that explain it better.

This PR is assuming that#1344 will be merged. As that is what triggered this issue.

Copy link

codecov-io commentedMar 3, 2021•
edited
Loading

Codecov Report

Merging#1352 (a7a0487) intomaster (96d2805) willnot change coverage.
The diff coverage is100.00%.

@@           Coverage Diff           @@##           master    #1352   +/-   ##=======================================  Coverage   80.21%   80.21%           =======================================  Files          73       73             Lines        3801     3801           =======================================  Hits         3049     3049             Misses        752      752

Flag	Coverage Δ
unit	`80.21% <100.00%> (ø)`

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

Impacted Files	Coverage Δ
gitlab/v4/objects/commits.py	`78.26% <100.00%> (ø)`
gitlab/v4/objects/deployments.py	`100.00% <100.00%> (ø)`
gitlab/v4/objects/jobs.py	`63.79% <100.00%> (ø)`
gitlab/v4/objects/pipelines.py	`88.00% <100.00%> (ø)`
gitlab/v4/objects/releases.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update96d2805...a7a0487. Read thecomment docs.

Copy link

MemberAuthor

JohnVillalovos commentedMar 3, 2021

@max-wittig Hopefully I have answered your question.

Copy link

MemberAuthor

JohnVillalovos commentedMar 15, 2021

Any more questions or comments about this?

nejch requested changes

Apr 17, 2021

View reviewed changes

Copy link

Member

nejch left a comment

Choose a reason for hiding this comment

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

Thanks@JohnVillalovos! Just a few quick comments.

gitlab/tests/objects/test_mro.py OutdatedShow resolvedHide resolved

Copy link

MemberAuthor

JohnVillalovos commentedApr 17, 2021

@nejch Thanks for the feedback. I'll try to get some time to work on this in the next few days.

Copy link

MemberAuthor

JohnVillalovos commentedApr 18, 2021

@nejch Updated. I removed the debug code and moved the comments into the docstring.

JohnVillalovos requested a review fromnejch

April 18, 2021 16:52

JohnVillalovos mentioned this pull request

Apr 23, 2021

Fix all issues reported by running: tox -e pep8 and enable pep8 as a linter check#1397

Merged

fix: add a check to ensure the MRO is correct

565d548

Add a check to ensure the MRO (Method Resolution Order) is correct for classes ingitlab.v4.objects when doing type-checking.An example of an incorrect definition:    class ProjectPipeline(RESTObject, RefreshMixin, ObjectDeleteMixin):                          ^^^^^^^^^^ This should be at the end.Correct way would be:    class ProjectPipeline(RefreshMixin, ObjectDeleteMixin, RESTObject):                                      Correctly at the end ^^^^^^^^^^Also fix classes which have the issue.

nejch reviewed

Apr 27, 2021

View reviewed changes

gitlab/tests/objects/test_mro.py OutdatedShow resolvedHide resolved