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

fix: avoid passing redundant arguments to API#1965

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

max-wittig merged 1 commit intomainfromfix/redundant-args-api

Apr 14, 2022

Merged

fix: avoid passing redundant arguments to API#1965

max-wittig merged 1 commit intomainfromfix/redundant-args-api

Apr 14, 2022

Conversation

Copy link

Member

nejch commentedApr 4, 2022

Closes#987.

This was already fixed inb9ecc9a but partially reintroduced with the globalretry_transient_errors feature.

fix: avoid passing redundant arguments to API

3431887

Copy link

codecov-commenter commentedApr 4, 2022

Codecov Report

Merging#1965 (3431887) intomain (0353bd4) willincrease coverage by0.00%.
The diff coverage is100.00%.

@@           Coverage Diff           @@##             main    #1965   +/-   ##=======================================  Coverage   92.56%   92.56%           =======================================  Files          78       78             Lines        4909     4910    +1     =======================================+ Hits         4544     4545    +1  Misses        365      365

Flag	Coverage Δ
cli_func_v4	`81.58% <100.00%> (+<0.01%)`	⬆️
py_func_v4	`80.12% <100.00%> (+<0.01%)`	⬆️
unit	`83.48% <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	`90.58% <100.00%> (+0.02%)`	⬆️

nejch requested a review fromJohnVillalovos

April 5, 2022 07:19

Copy link

Member

max-wittig commentedApr 6, 2022

@nejch Mhmm but where is thatretry_transient_errors is actually removed. I just see that it's set as a self attribute.

max-wittig assignednejch

Apr 6, 2022

Copy link

MemberAuthor

nejch commentedApr 6, 2022

@nejch Mhmm but where is thatretry_transient_errors is actually removed. I just see that it's set as a self attribute.

@max-wittig previously it would just get picked from the method's**kwargs via.get():

python-gitlab/gitlab/client.py

Lines 680 to 682 in3b0806e

	retry_transient_errors=kwargs.get(
	"retry_transient_errors",self.retry_transient_errors
	)

but all kwargs are passed as query parameters to the API because they're copied to params earlier:

python-gitlab/gitlab/client.py

Line 669 in3431887

utils.copy_dict(src=kwargs,dest=params)

I added it as an explicit argument to avoid that. I could have done.pop() from kwargs instead, but it would have to be done before that dict is copied, so I thought it'd be cleaner like this.