Skip to content

feat(client): introduce iterator=True and deprecate as_list=False in list() #2032

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

Merged
merged 1 commit into from
May 29, 2022

Conversation

JohnVillalovos
Copy link
Member

@JohnVillalovos JohnVillalovos commented May 29, 2022

as_list=False is confusing as it doesn't explain what is being
returned. Replace it with iterator=True which more clearly explains
to the user that an iterator/generator will be returned.

This maintains backward compatibility with as_list but does issue a
DeprecationWarning if as_list is set.

@JohnVillalovos JohnVillalovos force-pushed the jlvillal/i_hate_as_list branch from 7825972 to c678721 Compare May 29, 2022 18:52
@JohnVillalovos JohnVillalovos marked this pull request as draft May 29, 2022 19:01
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/i_hate_as_list branch from c678721 to b4ae128 Compare May 29, 2022 19:04
@JohnVillalovos JohnVillalovos marked this pull request as ready for review May 29, 2022 19:07
@JohnVillalovos JohnVillalovos requested a review from nejch May 29, 2022 19:08
@codecov-commenter
Copy link

Codecov Report

Merging #2032 (b4ae128) into main (5ae18d0) will decrease coverage by 0.01%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main    #2032      +/-   ##
==========================================
- Coverage   92.72%   92.71%   -0.02%     
==========================================
  Files          78       78              
  Lines        4947     4953       +6     
==========================================
+ Hits         4587     4592       +5     
- Misses        360      361       +1     
Flag Coverage Δ
cli_func_v4 81.46% <35.71%> (-0.04%) ⬇️
py_func_v4 80.37% <85.71%> (+<0.01%) ⬆️
unit 83.72% <35.71%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
gitlab/mixins.py 91.75% <ø> (ø)
gitlab/v4/objects/ldap.py 54.54% <ø> (ø)
gitlab/v4/objects/repositories.py 83.07% <ø> (ø)
gitlab/v4/objects/runners.py 98.27% <ø> (ø)
gitlab/v4/objects/users.py 95.94% <ø> (ø)
gitlab/v4/objects/merge_requests.py 84.78% <50.00%> (ø)
gitlab/client.py 90.68% <87.50%> (-0.10%) ⬇️
gitlab/v4/objects/milestones.py 100.00% <100.00%> (ø)

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.

Looks good @JohnVillalovos just a few minor comments.

Context for future readers:
https://github.com/python-gitlab/python-gitlab/pull/1956/files#r884302316

… in `list()`

`as_list=False` is confusing as it doesn't explain what is being
returned. Replace it with `iterator=True` which more clearly explains
to the user that an iterator/generator will be returned.

This maintains backward compatibility with `as_list` but does issue a
DeprecationWarning if `as_list` is set.
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/i_hate_as_list branch from b4ae128 to cdc6605 Compare May 29, 2022 22:51
@JohnVillalovos JohnVillalovos requested a review from nejch May 29, 2022 22:51
@JohnVillalovos JohnVillalovos changed the title fix: in list() change as_list=False to iterator=True feat(client): introduce iterator=True and deprecate as_list=False in list() May 29, 2022
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 @JohnVillalovos, makes sense to me, also makes it consistent with the iterator arg on the upcoming streaming responses PR then.

@nejch nejch enabled auto-merge May 29, 2022 23:30
@nejch nejch merged commit c51b538 into main May 29, 2022
@nejch nejch deleted the jlvillal/i_hate_as_list branch May 29, 2022 23:33
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.

3 participants