Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 12, 2023

Conversation

lmilbaum
Copy link
Contributor

@lmilbaum lmilbaum commented Dec 26, 2022

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
Copy link

codecov-commenter commented Dec 26, 2022

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
Copy link
Member

@nejch 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 @lmilbaum, I'm quite slow with the reviews here. Just a small note on keeping things simple for now.

.github/workflows/protocol.yml Outdated Show resolved Hide resolved
gitlab/protocols/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@JohnVillalovos JohnVillalovos left a comment

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?

gitlab/protocols/http_backend.py Outdated Show resolved Hide resolved
gitlab/protocols/http_backend_response.py Outdated Show resolved Hide resolved
@lmilbaum
Copy link
Contributor Author

lmilbaum commented Feb 7, 2023

/rerun-all

@JohnVillalovos
Copy link
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)

@lmilbaum
Copy link
Contributor Author

lmilbaum commented Feb 7, 2023

/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
Copy link
Member

nejch commented Feb 7, 2023

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
Copy link
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
Copy link
Member

nejch commented Feb 7, 2023

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

PEP 544 – Protocols: Structural subtyping (static duck typing)
@nejch nejch merged commit 4afeaff into python-gitlab:main Feb 12, 2023
@nejch
Copy link
Member

nejch commented Feb 12, 2023

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants