8000 feat: emit a warning when using a `list()` method returns max by JohnVillalovos · Pull Request #1875 · python-gitlab/python-gitlab · GitHub
[go: up one dir, main page]
Skip to content

feat: emit a warning when using a list() method returns max #1875

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 13, 2022
Merged

feat: emit a warning when using a list() method returns max #1875

merged 1 commit into from
Apr 13, 2022

Conversation

JohnVillalovos
Copy link
Member
@JohnVillalovos JohnVillalovos commented Feb 3, 2022 •
edited
Loading

A common cause of issues filed and questions raised is that a user
will call a list() method and only get 20 items. As this is the
default maximum of items that will be returned from a list() method.

To help with this we now emit a warning when the result from a
list() method is greater-than or equal to 20 (or the specified
per_page value) and the user is not using either all=True,
all=False, as_list=False, or page=X.

@codecov-commenter
Copy link
codecov-commenter commented Feb 3, 2022 •
edited
Loading

Codecov Report

Merging #1875 (1339d64) into main (5370979) will increase coverage by 0.00%.
The diff coverage is 96.15%.

@@           Coverage Diff           @@
##             main    #1875   +/-   ##
=======================================
  Coverage   92.56%   92.57%           
=======================================
  Files          78       78           
  Lines        4910     4930   +20     
=======================================
+ Hits         4545     4564   +19     
- Misses        365      366    +1
Flag Coverage Δ
cli_func_v4 81.52% <61.53%> (-0.07%) ⬇️
py_func_v4 80.46% <96.15%> (+0.34%) ⬆️
unit 83.50% <92.30%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
gitlab/client.py 90.76% <96.15%> (+0.19%) ⬆️

@JohnVillalovos JohnVillalovos force-pushed the jlvillal/list_warning branch 2 times, most recently from d6ef9fe to 9d1e6e8 Compare February 3, 2022 23:37
@JohnVillalovos JohnVillalovos marked this pull request as draft February 3, 2022 23:38
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/list_warning branch 3 times, most recently from 92da850 to c790f8c Compare February 4, 2022 17:09
@JohnVillalovos
Copy link
Member Author
JohnVillalovos commented Feb 4, 2022 •
edited
Loading

Example program using this:

$ cat show-it.py
#!/usr/bin/python3 -ttu
import gitlab

gl = gitlab.Gitlab(url="https://gitlab.com")
project = gl.projects.get("gitlab-org/gitlab")

mrs = project.mergerequests.list(state="opened")

Run of program:

$ ./show-it.py
..././show-it.py:7: UserWarning: Calling a `list()` method without specifying `all=True` or `as_list=False` will return a maximum of 20 items. Your query returned 20 of 1286 items. See https://python-gitlab.readthedocs.io/en/v3.1.1/api-usage.html#pagination for more details. If this was done intentionally, then this warning can be supressed by adding the argument `page=1` or `get_all=False` to the `list()` call.
  mrs = project.mergerequests.list(state="opened")

@JohnVillalovos JohnVillalovos marked this pull request as ready for review February 4, 2022 17:14
@JohnVillalovos JohnVillalovos requested a review from nejch February 4, 2022 17:14
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/list_warning branch 2 times, most recently from 0223989 to 63a6f1e Compare February 4, 2022 22:52
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/list_warning branch 5 times, most recently from ec10a57 to 9aa7a66 Compare February 5, 2022 17:52
@JohnVillalovos JohnVillalovos marked this pull request as draft February 5, 2022 18:41
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/list_warning branch 2 times, most recently from fb933c5 to 1fc8aa8 Compare February 6, 2022 19:07
nejch
nejch reviewed Feb 6, 2022
View reviewed changes
@JohnVillalovos JohnVillalovos marked this pull request as ready for review February 7, 2022 16:52
@JohnVillalovos JohnVillalovos requested a review from nejch February 7, 2022 16:52
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/list_warning branch 4 times, most recently from 3bd6d8d to 439e50b Compare February 14, 2022 16:47
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/list_warning branch from 439e50b to 0969cd8 Compare March 10, 2022 16:41
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/list_warning branch 2 times, most recently from f99cf74 to ac15b21 Compare March 27, 2022 14:31
@nejch
Copy link
Member
nejch commented Apr 2, 2022

Finally had a look around and here's some more valid use cases where we don't want to emit warnings on this - essentially whenever any kind of list filter is applied, I'd say. In those cases users mostly only care about the first item.

  1. Getting a user by username (can't use the GET endpoint): https://github.com/ansible-collections/community.general/blob/21ee4c84b7da0f5f540882b6d035c47da473db1f/plugins/modules/source_control/gitlab/gitlab_user.py#L521-L525
  2. Get group/project via loose name search: https://github.com/ansible-collections/community.general/blob/21ee4c84b7da0f5f540882b6d035c47da473db1f/plugins/modules/source_control/gitlab/gitlab_group_members.py#L180-L190
  3. Checking for existence of items: https://github.com/ansible-collections/community.general/blob/21ee4c84b7da0f5f540882b6d035c47da473db1f/plugins/modules/source_control/gitlab/gitlab_group.py#L282-L284

nejch
nejch reviewed Apr 7, 2022
View reviewed changes
@JohnVillalovos
Copy link
Member Author
JohnVillalovos commented Apr 11, 2022 •
edited
Loading

Answering without doing a lot of research on these:

Finally had a look around and here's some more valid use cases where we don't want to emit warnings on this - essentially whenever any kind of list filter is applied, I'd say. In those cases users mostly only care about the first item.

  1. Getting a user by username (can't use the GET endpoint): https://github.com/ansible-collections/community.general/blob/21ee4c84b7da0f5f540882b6d035c47da473db1f/plugins/modules/source_control/gitlab/gitlab_user.py#L521-L525

Warning will not be issued unless for some reason 20 items are returned by the list() call. In that case it is likely highlighting a real error. I have submitted a PR that adds a all=True for this.

  1. Get group/project via loose name search: https://github.com/ansible-collections/community.general/blob/21ee4c84b7da0f5f540882b6d035c47da473db1f/plugins/modules/source_control/gitlab/gitlab_group_members.py#L180-L190

Warning will not be issued unless for some reason 20 items are returned by the list() call. In that case it is likely highlighting a real error. I have submitted a PR that adds a all=True for this.

  1. Checking for existence of items: https://github.com/ansible-collections/community.general/blob/21ee4c84b7da0f5f540882b6d035c47da473db1f/plugins/modules/source_control/gitlab/gitlab_group.py#L282-L284

Yes a warning has a good chance of being produced for this one. I have submitted a PR that adds a all=False for this.

ansible-collections/community.general#4491

@JohnVillalovos JohnVillalovos force-pushed the jlvillal/list_warning branch from ac15b21 to 9117111 Compare April 12, 2022 13:19
@JohnVillalovos
feat: emit a warning when using a list() method returns max
1339d64 
A common cause of issues filed and questions raised is that a user
will call a `list()` method and only get 20 items. As this is the
default maximum of items that will be returned from a `list()` method.

To help with this we now emit a warning when the result from a
`list()` method is greater-than or equal to 20 (or the specified
`per_page` value) and the user is not using either `all=True`,
`all=False`, `as_list=False`, or `page=X`.
@JohnVillalovos JohnVillalovos force-pushed the jlvillal/list_warning branch from 9117111 to 1339d64 Compare April 12, 2022 13:24
@JohnVillalovos JohnVillalovos requested a review from nejch April 12, 2022 13:28
nejch
nejch approved these changes Apr 13, 2022
View reviewed changes
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 for this @JohnVillalovos! Took a while as I am really uneasy about eagerly sending warnings but this will definitely reduce the issues filed.

@nejch nejch merged commit 4d6f125 into main Apr 13, 2022
@nejch nejch deleted the jlvillal/list_warning branch April 13, 2022 05:50
@JohnVillalovos
Copy link
Member Author

Thanks for this @JohnVillalovos! Took a while as I am really uneasy about eagerly sending warnings but this will definitely reduce the issues filed.

Thanks @nejch ! I'm interested to see feedback on it once it starts getting used by people. I know when I tried it on my code I found spots where I was like "whoops!".

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

@max-wittig max-wittig Awaiting requested review from max-wittig

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JohnVillalovos @codecov-commenter @nejch
0