Fix handling of Skip and Start parameters for pagination #98
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The way we treated Skip and Start parameters was inconsistent and wrong. In some calls we send both parameters (s, start, S). This causes and undefined behaviour at Gerrit itself. Additional the Skip/Start parameters are numbers and not strings. Here we fixed the Go types. Thanks a lot to @michaeldorner for his initial work in #48 Related: - #48
* master: Groups: Add _more_groups attribute to GroupInfo Fix #92: Add pagination support for SuggestAccount
|
@dmitshur If you find some spare time, it would be great to get your pair of eyes and a bit of feedback. Thanks mate! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The way how we handle pagination is not optimal, partially broken, and sometimes even inconsistent with the current (v3.4.1) Gerrit REST API docs.
Not optimal
Parameters are defined as strings in some Options.
It should be an integer.
Partially broken
In some calls, you can set a
SkipandStartparameters.On Gerrit Server Side this is causing a (we assume) undefined behavior.
However, the call with both parameters
sandstartset will not succeed.Inconsistent with the current (v3.4.1) Gerrit REST API docs
In the Gerrit docs, the setting is sometimes named Start. Sometimes Skip.
On go-gerrit side, we use kinda both and something the other version per call.
This Pull Request is ...
Breaking change
This PR is introducing a breaking change.
I believe it is worth doing it, because
Special notice
This PR is a follow-up from the work of @michaeldorner in #48.
After the merge
I aim to create a new library release including a migration guide for people who are adopting this library.