Skip to content

feat(backends): use PEP544 protocols for structural subtyping - #2442

Merged
nejch merged 1 commit into
python-gitlab:mainfrom
lmilbaum:protocol
Feb 12, 2023
Merged

feat(backends): use PEP544 protocols for structural subtyping#2442
nejch merged 1 commit into
python-gitlab:mainfrom
lmilbaum:protocol

Conversation

@lmilbaum

@lmilbaum lmilbaum commented Dec 26, 2022 •
edited
Loading

Copy link
Copy Markdown

The purpose of this change is to track api changes. For example: package versioning and breaking change announcement in case of protocol change.
This is MVP implementation to be used by #2435
Haven't figured out yet how to implement unit tests for a protocol and its value.

@lmilbaum lmilbaum self-assigned this Dec 26, 2022
@lmilbaum
lmilbaum force-pushed the protocol branch 2 times, most recently from 3feb3b7 to 54bbb3f Compare December 26, 2022 05:10
@codecov-commenter

codecov-commenter commented Dec 26, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2442 (722312e) into main (3d7ca1c) will increase coverage by 4.29%.
The diff coverage is 84.21%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #2442      +/-   ##
==========================================
+ Coverage   91.85%   96.15%   +4.29%     
==========================================
  Files          86       87       +1     
  Lines        5646     5663      +17     
==========================================
+ Hits         5186     5445     +259     
+ Misses        460      218     -242
Flag Coverage Δ
api_func_v4 82.51% <84.21%> (?)
cli_func_v4 82.97% <84.21%> (-0.01%) ⬇️
unit 87.60% <84.21%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
gitlab/_backends/protocol.py 81.25% <81.25%> (ø)
gitlab/_backends/requests_backend.py 96.00% <100.00%> (+0.08%) ⬆️
gitlab/v4/objects/merge_request_approvals.py 98.75% <0.00%> (+1.25%) ⬆️
gitlab/utils.py 98.59% <0.00%> (+1.40%) ⬆️
gitlab/v4/objects/members.py 94.82% <0.00%> (+1.72%) ⬆️
gitlab/client.py 98.79% <0.00%> (+3.42%) ⬆️
gitlab/types.py 98.21% <0.00%> (+3.57%) ⬆️
gitlab/v4/objects/notes.py 94.28% <0.00%> (+3.80%) ⬆️
gitlab/v4/objects/branches.py 100.00% <0.00%> (+4.34%) ⬆️
gitlab/v4/objects/events.py 98.85% <0.00%> (+4.59%) ⬆️
... and 28 more

@lmilbaum
lmilbaum marked this pull request as ready for review December 26, 2022 05:32
@lmilbaum
lmilbaum force-pushed the protocol branch 7 times, most recently from 3e66763 to 99f321a Compare December 26, 2022 17:30
nejch
nejch reviewed Jan 23, 2023
View reviewed changes
Comment thread .github/workflows/protocol.yml Outdated
@lmilbaum
lmilbaum force-pushed the protocol branch 2 times, most recently from a34c5c0 to 0fde7ca Compare January 24, 2023 04:59
@lmilbaum
lmilbaum requested a review from nejch January 24, 2023 05:13
nejch
nejch requested changes Feb 5, 2023
View reviewed changes

@nejch nejch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @lmilbaum, I'm quite slow with the reviews here. Just a small note on keeping things simple for now.

Comment thread .github/workflows/protocol.yml Outdated
Comment thread gitlab/protocols/__init__.py Outdated
@nejch nejch mentioned this pull request Feb 5, 2023
Closed
JohnVillalovos
JohnVillalovos reviewed Feb 6, 2023
View reviewed changes

@JohnVillalovos JohnVillalovos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks. Can the commit message have some details on the "why" of this PR?

Comment thread gitlab/protocols/http_backend.py Outdated
Comment thread gitlab/protocols/http_backend_response.py Outdated
@lmilbaum
lmilbaum force-pushed the protocol branch 5 times, most recently from 3e6e9e5 to acf2b49 Compare February 7, 2023 04:58
@lmilbaum

lmilbaum commented Feb 7, 2023

Copy link
Copy Markdown
Author

/rerun-all

@JohnVillalovos

JohnVillalovos commented Feb 7, 2023

Copy link
Copy Markdown
Member

Thanks. Can the commit message have some details on the "why" of this PR?

I still would like to see the description be put into the commit message.

At the moment all I see is:

$ git show --no-patch
commit 958d35d3c3d1755edf651cb15f5d1397ca571cc6 (HEAD -> refs/heads/protocol, refs/remotes/origin/protocol)
Author: Liora Milbaum <liora@lmb.co.il>
Date:   Mon Dec 26 06:39:58 2022 +0200

    feat: PEP 544 – Protocols: Structural subtyping (static duck typing)

JohnVillalovos
JohnVillalovos reviewed Feb 7, 2023
View reviewed changes
Comment thread gitlab/_backends/protocol.py
@lmilbaum

lmilbaum commented Feb 7, 2023

Copy link
Copy Markdown
Author

/rerun-all

@nejch nejch changed the title feat: PEP 544 – Protocols: Structural subtyping (static duck typing) feat(backends): use PEP544 protocols for structural subtyping Feb 7, 2023
@nejch

nejch commented Feb 7, 2023

Copy link
Copy Markdown
Member

Thanks. Can the commit message have some details on the "why" of this PR?

I still would like to see the description be put into the commit message.

At the moment all I see is:

As an alternative we can squash-merge and it will link to the PR which has much more context than we would usually put in commit messages. WDYT @JohnVillalovos

@JohnVillalovos

JohnVillalovos commented Feb 7, 2023

Copy link
Copy Markdown
Member

As an alternative we can squash-merge and it will link to the PR which has much more context than we would usually put in commit messages. WDYT @JohnVillalovos

That is okay if it is in the commit message. I want the details in the Git log. Not only on GitHub. What if the project gets moved to GitLab in the future. That information can disappear if it isn't in the git repository.

Seems relatively easy to take the description above and put it in the commit message. But maybe I am missing something?

@nejch

nejch commented Feb 7, 2023

Copy link
Copy Markdown
Member

Sure, we can maybe copy it during the squash merge in the description field and it'll appear in the body. I've already sneakily edited the PR title a bit 😺

GitLab's importer is actually quite good these days so it would preserve the commit-to-MR associations, I personally like that over long commit bodies as it includes the review context but that's subjective of course.

I'll approve for now, and see if there's anything else to cover :)

nejch
nejch reviewed Feb 7, 2023
View reviewed changes
Comment thread gitlab/_backends/protocol.py
@nejch nejch mentioned this pull request Feb 7, 2023
Closed
Liora Milbaum @nejch
feat: protocol backend classes to indentify breaking api changes
722312e 
PEP 544 – Protocols: Structural subtyping (static duck typing)
@nejch
nejch merged commit 4afeaff into python-gitlab:main Feb 12, 2023
@nejch

nejch commented Feb 12, 2023

Copy link
Copy Markdown
Member

As an alternative we can squash-merge and it will link to the PR which has much more context than we would usually put in commit messages. WDYT @JohnVillalovos

That is okay if it is in the commit message. I want the details in the Git log. Not only on GitHub. What if the project gets moved to GitLab in the future. That information can disappear if it isn't in the git repository.

Seems relatively easy to take the description above and put it in the commit message. But maybe I am missing something?

I added the description in the body and reworded a bit during merge, 4afeaff.

@lmilbaum
lmilbaum deleted the protocol branch February 12, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nejch nejch nejch approved these changes
@JohnVillalovos JohnVillalovos Awaiting requested review from JohnVillalovos

Assignees

@lmilbaum lmilbaum

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lmilbaum @codecov-commenter @JohnVillalovos @nejch