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(client): add http_patch method #2471

Merged
merged 1 commit into from Feb 5, 2023
Merged

feat(client): add http_patch method #2471

merged 1 commit into from Feb 5, 2023

Conversation

JohnVillalovos
Copy link
Member

In order to support some new API calls we need to support the HTTP PATCH method.

Closes: #2469

@JohnVillalovos JohnVillalovos marked this pull request as draft February 3, 2023 01:34
@JohnVillalovos
Copy link
Member Author

@nejch Just wanted to get the ball rolling on this. Not sure how much should be in the PR.

Some unit tests are going to be wanted. Not sure about functional tests.

@lmilbaum
Copy link
Contributor

lmilbaum commented Feb 3, 2023

How about placing the function in the backend?

@JohnVillalovos
Copy link
Member Author

How about placing the function in the backend?

Since I don't know much about the backend, can you explain why? I'm doing it like all the other http_* methods which currently exist.

@lmilbaum
Copy link
Contributor

lmilbaum commented Feb 3, 2023

How about placing the function in the backend?

Since I don't know much about the backend, can you explain why? I'm doing it like all the other http_* methods which currently exist.

Moving the http_* functions to the backend has already started. It will save a future work of moving this one as well and there will be no need to change the function signature.

@JohnVillalovos
Copy link
Member Author

JohnVillalovos commented Feb 3, 2023

Moving the http_* functions to the backend has already started. It will save a future work of moving this one as well and there will be no need to change the function signature.

Sorry. I haven't looked into the back-end code. Can you explain how that will work? I don't really see much of anything in the back-end. I don't see any of the http_* methods there. EDIT: I see http_request

If all the other http_* methods need to be moved then I'm not sure how this one extra function is going to add much to the work required.

@lmilbaum
Copy link
Contributor

lmilbaum commented Feb 3, 2023

Moving the http_* functions to the backend has already started. It will save a future work of moving this one as well and there will be no need to change the function signature.

Sorry. I haven't looked into the back-end code. Can you explain how that will work? I don't really see much of anything in the back-end. I don't see any of the http_* methods there. EDIT: I see http_request

If all the other http_* methods need to be moved then I'm not sure how this one extra function is going to add much to the work required.

You might want to take a look at #2435

@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2023

Codecov Report

Merging #2471 (b27f4ee) into main (0867564) will increase coverage by 0.00%.
The diff coverage is 100.00%.

📣 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    #2471   +/-   ##
=======================================
  Coverage   96.18%   96.19%           
=======================================
  Files          86       86           
  Lines        5635     5644    +9     
=======================================
+ Hits         5420     5429    +9     
  Misses        215      215           
Flag Coverage Δ
api_func_v4 82.54% <11.11%> (-0.12%) ⬇️
cli_func_v4 82.74% <11.11%> (-0.12%) ⬇️
unit 87.65% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
gitlab/client.py 98.79% <100.00%> (+0.02%) ⬆️

@nejch
Copy link
Member

nejch commented Feb 3, 2023

@JohnVillalovos, I think this could be a feat as people can already use it for unimplemented endpoints after merge.

Moving the http_* functions to the backend has already started. It will save a future work of moving this one as well and there will be no need to change the function signature.

Sorry. I haven't looked into the back-end code. Can you explain how that will work? I don't really see much of anything in the back-end. I don't see any of the http_* methods there. EDIT: I see http_request
If all the other http_* methods need to be moved then I'm not sure how this one extra function is going to add much to the work required.

You might want to take a look at #2435

@lmilbaum in any case we will have these user-facing helper methods on the client, preferably backend-agnostic (https://python-gitlab.readthedocs.io/en/stable/api-levels.html#lower-level-api-http-methods). They help users work around endpoints we haven't implemented in the library.

I hope if we get this right only http_request and the associated typing will need a significant backend-specific implementation and the rest can wrap around it like we do now.

gitlab/client.py Show resolved Hide resolved
@lmilbaum
Copy link
Contributor

lmilbaum commented Feb 3, 2023

@JohnVillalovos, I think this could be a feat as people can already use it for unimplemented endpoints after merge.

Moving the http_* functions to the backend has already started. It will save a future work of moving this one as well and there will be no need to change the function signature.

Sorry. I haven't looked into the back-end code. Can you explain how that will work? I don't really see much of anything in the back-end. I don't see any of the http_* methods there. EDIT: I see http_request
If all the other http_* methods need to be moved then I'm not sure how this one extra function is going to add much to the work required.

You might want to take a look at #2435

@lmilbaum in any case we will have these user-facing helper methods on the client, preferably backend-agnostic (https://python-gitlab.readthedocs.io/en/stable/api-levels.html#lower-level-api-http-methods). They help users work around endpoints we haven't implemented in the library.

I hope if we get this right only http_request and the associated typing will need a significant backend-specific implementation and the rest can wrap around it like we do now.

I am not following what you are trying to say. In any case, maybe switching the return type in a similar way I've done in the PR #2435.

In order to support some new API calls we need to support the HTTP `PATCH` method.

Closes: #2469
@JohnVillalovos JohnVillalovos marked this pull request as ready for review February 4, 2023 01:16
@nejch nejch changed the title chore: add http_patch method feat: add http_patch method Feb 5, 2023
@nejch
Copy link
Member

nejch commented Feb 5, 2023

I am not following what you are trying to say. In any case, maybe switching the return type in a similar way I've done in the PR #2435.

@lmilbaum since this PR is a separate concern, I think we can do that after, I can also rebase your PR with http_patch to avoid more rebase issues on your end. I think we can go ahead with this one just to unblock users who want to update protected branches in #2390.

@nejch nejch changed the title feat: add http_patch method feat(client): add http_patch method Feb 5, 2023
@nejch nejch merged commit f711d9e into main Feb 5, 2023
@nejch nejch deleted the jlvillal/http_patch branch February 5, 2023 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFE: Add support for PATCH requests. Create a http_patch() method
4 participants