Skip to content
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.

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

Add options to GetIssuesForSprint #353

Closed
wants to merge 1 commit into from
Closed

Conversation

oriser
Copy link

@oriser oriser commented Feb 13, 2021

Description

Adding query options to GetIssuesForSprint and GetIssueWithContext

Information that is useful here:

  • The What: Adding missing query options to GetIssuesForSprint and GetIssueWithContext
  • The Why: To be able to use Jira provided filtering by API
  • Type of change: Missing API query
  • Breaking change: Yes. Whoever currently uses GetIssuesForSprint and will update to this version, will need to pass nil (or real query if he wish)
  • Related to an issue: closes No query options for GetIssuesForSprint #352
  • Jira Version + Type: Jira cloud

Example:

Let us know how users can use or test this functionality.

issues, _, err := j.client.Sprint.GetIssuesForSprint(sprint.ID, &jira.GetIssuesForSprintOptions{
	Jql: fmt.Sprintf(`assignee="%s"`, j.assignee),
})

Checklist

@andygrunwald
Copy link
Owner

The CHange is looking good. Thanks @oriser!

I will keep it open for a few days to provide the opportunity for other contributors to review it.
Especially, because this is a breaking change to the existing API (but I think it is a good + manageable one).

@gueldenstone
Copy link

Hi, sorry if I'm wrong. Maybe it's a good idea to make a new function along the lines of "GetIssuesForSprintWithOptions", so it would not be a breaking change and possibly add a "MaxResults" option to the options? Then other options, such as "MaxResults" and "StartAt" could also be implemented?

@benjivesterby
Copy link
Contributor

@gueldenstone has a good point. One way this could be done is by something like this

Update the SearchOptions struct to implement the query.Encoder interface from the query package using this method EncodeValues(key string, v *url.Values) error

Then add a new struct type for the JQL string you want added and also implement the query.Encoder interface from the query package using the method EncodeValues(key string, v *url.Values) error

Then update the API for GetIssuesForSprintWithContext so that it looks like this

func (s *SprintService) GetIssuesForSprintWithContext(ctx context.Context, sprintID int, encoders ...query.Encoder) ([]Issue, *Response, error) {
...
url := apiEndpoint
for _, encoder := range encoders {
 url, err := addOptions(url, encoder)
}
...

Since the encoders parameter would be variadic it would not break the API for existing users and allow you to extend the functionality. It would also make it flexible to be able to handle additional encoders in the future.

Copy link
Contributor

@benjivesterby benjivesterby left a comment

Choose a reason for hiding this comment

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

This change would also require the implementation of the query.Encoder interface for the SearchOptions struct which is not in the current set of changed files.

See comment: #353 (comment)

@oriser
Copy link
Author

oriser commented Feb 24, 2021

@benjivesterby I can do it NP, but IMHO it better like the original PR, as I saw the rest of the calls are look like this and I think it better to keep the same conventions..
This breaking change is pretty minor as it will only require to add ,nil to the function call.

Copy link
Contributor

@benjivesterby benjivesterby left a comment

Choose a reason for hiding this comment

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

After reviewing the query library used the Encoder option is not viable. I do not like the fact that this breaks the API as it will likely affect a number of users. I would prefer that the parameter added was variadic in nature and perhaps implemented a type defined in this package for example.

type QueryValues map[string]string

This way multiple query values could be passed while keeping the initial implementation if a user doesn't update their function signature. I'm open to suggestions but I feel like breaking the API for this change is unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No query options for GetIssuesForSprint
4 participants