Skip to content

Updated S parameter according to the Gerrit API #48

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

Closed
wants to merge 7 commits into from
Closed

Updated S parameter according to the Gerrit API #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0bc5630
changed all occurrences of the `S` parameter according to the Gerrit …
michaeldorner Apr 13, 2018
1f36d59
fixed format issue
michaeldorner Apr 13, 2018
a47fe1b
added change skip/start behaviour
michaeldorner May 10, 2020
c23b0aa
corrected S parameter according to Gerrit API documentation
michaeldorner May 10, 2020
0f32319
fixed logic for Skip/Start check
michaeldorner May 10, 2020
8871cec
added parameter value output
michaeldorner May 11, 2020
dbb8e80
fixed small typo for the parameter value output (start and skip)
michaeldorner May 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion changes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ type QueryOptions struct {
type QueryChangeOptions struct {
QueryOptions

// The S or start query parameter can be supplied to skip a number of changes from the list.
// The S or start query parameter can be supplied to skip a number of changes from the list. Only one of the two parameters can be set at a time.
Skip int `url:"S,omitempty"`
Start int `url:"start,omitempty"`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Author

@michaeldorner michaeldorner Apr 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, give me some days.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any news here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, but on my desk. :)


Expand All @@ -391,6 +391,10 @@ type ChangeOptions struct {
func (s *ChangesService) QueryChanges(opt *QueryChangeOptions) (*[]ChangeInfo, *Response, error) {
u := "changes/"

if !(opt.Skip == opt.Start || opt.Skip == 0 || opt.Start == 0) {
return nil, nil, fmt.Errorf("the query parameters `skip`/`S`(=%v) and `start` (=%v) are conflicting", opt.Skip, opt.Start)
}

u, err := addOptions(u, opt)
if err != nil {
return nil, nil, err
Expand Down
4 changes: 2 additions & 2 deletions projects.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ type ProjectBaseOptions struct {
Limit int `url:"n,omitempty"`

// Skip the given number of branches from the beginning of the list.
Skip string `url:"s,omitempty"`
Skip string `url:"S,omitempty"`
}

// ProjectOptions specifies the parameters to the ProjectsService.ListProjects.
Expand All @@ -193,7 +193,7 @@ type ProjectOptions struct {
Regex string `url:"r,omitempty"`

// Skip the given number of projects from the beginning of the list.
Skip string `url:"S,omitempty"`
Start string `url:"start,omitempty"`

// Limit the results to those projects that match the specified substring.
Substring string `url:"m,omitempty"`
Expand Down
2 changes: 1 addition & 1 deletion projects_branch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type BranchOptions struct {
Limit int `url:"n,omitempty"`

// Skip the given number of branches from the beginning of the list.
Skip string `url:"s,omitempty"`
Skip string `url:"S,omitempty"`

// Substring limits the results to those projects that match the specified substring.
Substring string `url:"m,omitempty"`
Expand Down