Skip to content

fix: handle situation where gitlab.com does not return values #1773

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
Dec 31, 2021
Merged

fix: handle situation where gitlab.com does not return values #1773

merged 1 commit into from
Dec 31, 2021

Conversation

JohnVillalovos
Copy link
Member

If a query returns more than 10,000 records than the following values
are NOT returned:
x.total_pages
x.total

Modify the code to allow no value to be set for these values. If there
is not a value returned the functions will now return None.

https://docs.gitlab.com/ee/user/gitlab_com/index.html#pagination-response-headers

Closes #1686

@JohnVillalovos JohnVillalovos requested a review from nejch December 22, 2021 22:14
@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2021 •
edited
Loading

Codecov Report

Merging #1773 (cb824a4) into main (501f9a1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1773   +/-   ##
=======================================
  Coverage   91.96%   91.97%           
=======================================
  Files          76       76           
  Lines        4754     4758    +4     
=======================================
+ Hits         4372     4376    +4     
  Misses        382      382
Flag Coverage Δ
cli_func_v4 81.21% <62.50%> (-0.07%) ⬇️
py_func_v4 80.62% <62.50%> (-0.07%) ⬇️
unit 83.18% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
gitlab/base.py 90.22% <100.00%> (ø)
gitlab/client.py 90.10% <100.00%> (+0.10%) ⬆️

@JohnVillalovos JohnVillalovos force-pushed the jlvillal/pagination branch 2 times, most recently from ca2e8e0 to 971a617 Compare December 22, 2021 22:38
nejch
nejch reviewed Dec 25, 2021
View reviewed changes
@nejch
Copy link
Member

nejch commented Dec 28, 2021

LGTM I'd just like to reproduce #1686 in some tests first with an xfail (e.g. I'll see if I can get responses to match the presence/absence of those headers). Are you ok to wait and rebase @JohnVillalovos (unless you want to take a look at it yourself but you already have a few PRs :D)

@JohnVillalovos
Copy link
Member Author

LGTM I'd just like to reproduce #1686 in some tests first with an xfail (e.g. I'll see if I can get responses to match the presence/absence of those headers). Are you ok to wait and rebase @JohnVillalovos (unless you want to take a look at it yourself but you already have a few PRs :D)

Works for me. Thanks!

@nejch
Copy link
Member

nejch commented Dec 30, 2021

I have #1788 ready for this and I also saw I was being overly pedantic as we're already doing the same with one-liners 😅 :

python-gitlab/gitlab/client.py

Lines 945 to 959 in c764bee

@property
def prev_page(self) -> Optional[int]:
"""The previous page number.
If None, the current page is the first.
"""
return int(self._prev_page) if self._prev_page else None
@property
def next_page(self) -> Optional[int]:
"""The next page number.
If None, the current page is the last.
"""
return int(self._next_page) if self._next_page else None

@JohnVillalovos JohnVillalovos force-pushed the jlvillal/pagination branch 2 times, most recently from 4a17937 to c308c30 Compare December 30, 2021 19:32
@JohnVillalovos JohnVillalovos requested a review from nejch December 30, 2021 19:33
@JohnVillalovos
Copy link
Member Author

I have #1788 ready for this and I also saw I was being overly pedantic as we're already doing the same with one-liners 😅 :

Thanks. I reviewed and merged #1788. Then rebased this PR. Was surprised that if an xfail marked test passes it doesn't cause things to fail 😕 Removed the xfail, and it is working as expected now.

@JohnVillalovos
fix: handle situation where GitLab does not return values
cb824a4 
If a query returns more than 10,000 records than the following values
are NOT returned:
  x.total_pages
  x.total

Modify the code to allow no value to be set for these values. If there
is not a value returned the functions will now return None.

Update unit test so no longer `xfail`

https://docs.gitlab.com/ee/user/gitlab_com/index.html#pagination-response-headers

Closes #1686
@JohnVillalovos
Copy link
Member Author

Was surprised that if an xfail marked test passes it doesn't cause things to fail 😕

I figured that out and added xfail_strict to the pytest configuration. Now if an xfail test passes it will cause the test run to fail.

@nejch nejch merged commit a3eafab into main Dec 31, 2021
@nejch nejch deleted the jlvillal/pagination branch December 31, 2021 20:50
This was referenced Jan 4, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nejch nejch Awaiting requested review from nejch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@JohnVillalovos @codecov-commenter @nejch