Updated S parameter according to the Gerrit API #48

michaeldorner · 2018-04-13T09:43:53Z

changed all occurrences of the S parameter according to the Gerrit API ("The S or start query parameter can be supplied to skip a number of changes from the list.")

…API ("The `S` or `start` query parameter can be supplied to skip a number of changes from the list.")

andygrunwald · 2018-04-13T09:51:23Z

changes.go

@@ -365,8 +365,7 @@ type QueryChangeOptions struct {
 	QueryOptions

 	// The S or start query parameter can be supplied to skip a number of changes from the list.
-	Skip  int `url:"S,omitempty"`
-	Start int `url:"start,omitempty"`


This is a breaking change here. If the start parameter is not filled, it is dropped anyway (regarding omitempty.
On the other side, it is consistent to only go with the S parameter instead offering both.
What do you think @opalmer and @shurcooL ?

I think there is an unexpecting behaviour if both fields are used. This is why I suggested to remove this uncertainty. And to be honest, I do not like the Gerrit parameter name "start" because it is very misleading: Gerrit skips the first n values, but does not start at nth value - it should be called start_after or just skip.

So here's my opinion:

Field should be named Skip since that mirrors the name used in other option structure.

Technically speaking Skip should actually be named Start since that mirrors what Gerrit's own documentation mentions. However, we shouldn't change that now because that would break a whole lot of code.

For now we should keep Start and Skip and:

Return an error if both are used. @michaeldorner's right that the current situation is a bit unexpected and we don't really know what the behavior is as a result. However....

If Start is used and Skip is not, set Skip = Start (and only have the json tags on Skip). That will keep existing code from breaking immediately.

Go does have a way of documenting deprecation: https://blog.golang.org/godoc-documenting-go-code

Sometimes a struct field, function, type, or even a whole package becomes redundant or unnecessary, but must be kept for compatibility with existing programs. To signal that an identifier should not be used, add a paragraph to its doc comment that begins with "Deprecated:" followed by some information about the deprecation.

The above suggestions seems like it would fix the odd behaviors, not break existing code unless that code is already broken and make it clear what the path for migration is. I'd probably be more inclined to break it if needed to be changed everywhere or there were no other options.

Thoughts?

Sounds good! @michaeldorner Would you mind changing the PR in this way @opalmer suggested?

Sure, give me some days.

Any news here?

Not yet, but on my desk. :)

codecov-io · 2018-04-13T09:52:19Z

Codecov Report

Merging #48 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #48   +/-   ##
=======================================
  Coverage   21.23%   21.23%           
=======================================
  Files          21       21           
  Lines        1775     1775           
=======================================
  Hits          377      377           
  Misses       1353     1353           
  Partials       45       45

Impacted Files	Coverage Δ
changes.go	`16.14% <ø> (ø)`
projects.go	`17.43% <ø> (ø)`
projects_branch.go	`0.00% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8fdb823...0f32319. Read the comment docs.

andygrunwald · 2020-05-03T13:29:48Z

Hey @michaeldorner,
any news on this? Anything I can help with?

michaeldorner · 2020-05-07T13:32:09Z

Sorry for the delay and even more sorry for forgetting about it. I will fix it as soon as possible (hopefully on the weekend).

michaeldorner · 2020-05-10T12:23:45Z

With the current solution, go-gerrit checks if Skip and Start are conflicting:

Both zero or the same value are fine
Either one parameter is 0 (default case)

The other skip parameters I changed accordingly to the documentation: There is not start parameter for projects.go and projects_branches.go.

What do you think?

andygrunwald · 2020-05-17T08:48:24Z

@dmitshur @opalmer Any opinions on this? What do you think?

andygrunwald · 2021-09-15T19:32:09Z

Maybe I made an implementation mistake with 101051e because of #48

I will fix this in the next days and add proper testing.

@michaeldorner

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

andygrunwald · 2021-09-19T12:19:27Z

Thanks a lot, @michaeldorner for your work on this.
I picked this up, tested it through, and added a bit more stuff around it.
The "new version" is available in #98

I will close this and we continue in #98.
If you disagree and you think there is more on this PR, let me know and I will re-open it.

michaeldorner added 2 commits April 13, 2018 11:41

changed all occurrences of the S parameter according to the Gerrit …

0bc5630

…API ("The `S` or `start` query parameter can be supplied to skip a number of changes from the list.")

fixed format issue

1f36d59

andygrunwald reviewed Apr 13, 2018

View reviewed changes

michaeldorner added 3 commits May 10, 2020 14:16

added change skip/start behaviour

a47fe1b

corrected S parameter according to Gerrit API documentation

c23b0aa

fixed logic for Skip/Start check

0f32319

michaeldorner added 2 commits May 11, 2020 09:00

added parameter value output

8871cec

fixed small typo for the parameter value output (start and skip)

dbb8e80

andygrunwald mentioned this pull request Aug 21, 2021

fix suggertaccout 500 limit #92

Closed

andygrunwald mentioned this pull request Sep 19, 2021

Fix handling of Skip and Start parameters for pagination #98

Open

andygrunwald closed this Sep 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated S parameter according to the Gerrit API #48

Updated S parameter according to the Gerrit API #48

michaeldorner commented Apr 13, 2018

andygrunwald Apr 13, 2018

michaeldorner Apr 13, 2018 •

edited

Loading

opalmer Apr 13, 2018

andygrunwald Apr 13, 2018

michaeldorner Apr 17, 2018

andygrunwald Aug 8, 2018

michaeldorner Aug 24, 2018

codecov-io commented Apr 13, 2018 •

edited

Loading

andygrunwald commented May 3, 2020

michaeldorner commented May 7, 2020

michaeldorner commented May 10, 2020 •

edited

Loading

andygrunwald commented May 17, 2020

andygrunwald commented Sep 15, 2021

andygrunwald commented Sep 19, 2021

Updated S parameter according to the Gerrit API #48

Updated S parameter according to the Gerrit API #48

Conversation

michaeldorner commented Apr 13, 2018

andygrunwald Apr 13, 2018

Choose a reason for hiding this comment

michaeldorner Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

opalmer Apr 13, 2018

Choose a reason for hiding this comment

andygrunwald Apr 13, 2018

Choose a reason for hiding this comment

michaeldorner Apr 17, 2018

Choose a reason for hiding this comment

andygrunwald Aug 8, 2018

Choose a reason for hiding this comment

michaeldorner Aug 24, 2018

Choose a reason for hiding this comment

codecov-io commented Apr 13, 2018 • edited Loading

Codecov Report

andygrunwald commented May 3, 2020

michaeldorner commented May 7, 2020

michaeldorner commented May 10, 2020 • edited Loading

andygrunwald commented May 17, 2020

andygrunwald commented Sep 15, 2021

andygrunwald commented Sep 19, 2021

michaeldorner Apr 13, 2018 •

edited

Loading

codecov-io commented Apr 13, 2018 •

edited

Loading

michaeldorner commented May 10, 2020 •

edited

Loading