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

#368 Added warningMessages to issue.search() response #369

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

manuelbcd
Copy link
Contributor

@manuelbcd manuelbcd commented Mar 23, 2021

Description

When using Issue.Search(...) response can contain optional field "WarningMessages" that was not implemented in the response struct.

This feature allows to manage possible WarningMessages since jql errors are not rendered as errors but as warning messages.
Without this option, you are unable to find out if your jql query failed because i.e. specified user does not exists.
More information in #368

Information that is useful here:

Example:

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

chunk, resp, warningMessages, err := client.Issue.Search(searchString, opt)
if err != nil {
	return nil, err
}
if len(warningMessages) > 0 {
  fmt.Printf("Warning messages in response:\n")
  for i, warn := range warningMessages {
	fmt.Printf("Warning %d: %s\n", i, warn)
  }
}

Checklist

@manuelbcd manuelbcd force-pushed the feature/warningmessages-searches branch 3 times, most recently from a0fcac8 to 660f411 Compare March 23, 2021 23:27
@manuelbcd manuelbcd force-pushed the feature/warningmessages-searches branch from 660f411 to 829c8e1 Compare March 23, 2021 23:28
@kusuriya
Copy link

LGTM, but we may want to have some notification around merging it. It looks like it changes the behavior around search as defined in the code that uses the library and could possibly break peoples code unless they handle for the new output variable. thoughts @andygrunwald

@manuelbcd
Copy link
Contributor Author

Yes @kusuriya, i do agree. Im worried because im not sure if is worth adding this feature while breaking code compatibility. Actually I was asking about that in #368

If someone can give it a thought... maybe there could be a different approach

@andygrunwald
Copy link
Owner

Thanks for the effort, the PR, and for the enhancement.
Indeed, the current implementation is breaking the compatibility.
We have a few PRs open that will break the existing interfaces, and this is the main reason why those doesn't have been merged yet.
For quite some time, we are talking about an official v2, that fixes many of these implementation details. I see two options here:

  1. Keeping this PR for v2 (no timeline yet)
  2. Changing the implementation into a compatible way

About 2), one idea would be to create a new Search function that supports these warning messages.
When a new Search functionality is created, I would even suggest changing the type that is returned to something like

type SearchResult struct {
    Issues []issue ...
    WarningMessage []messages ...
    ....
}

By this, we would not break the compatibility in the future, if a new field is part of the structure.
I am curious about what you think and your feedback.

@manuelbcd
Copy link
Contributor Author

Thanks for asking. In my opinion, adding a new function just to bring the ability to access to hypothetical warning messages could be a bit complex for library consumers... I bet for the 1st one, implement it into v2 refining implementation details first.
However if we receive more issues and votes to add the feature to v1, we can always add this third function in the future (in 3 weeks, in 3 months, it does not matter).

@andygrunwald
Copy link
Owner

Hey,

I am very sorry that this pull request has been open for a long time with no final feedback or merge/decline. We work on this project in our spare time, and sometimes, other priorities take over. This is the typical open source dilemma.

However, there is news: We are kicking off v2 of this library 🚀

To provide visibility, we created the Road to v2 Milestone and calling for your feedback in #489

The development will take some time; however, I hope you can benefit from the changes.
If you seek priority development for your pull request + you like to sponsor it, please contact me.

What does this mean for my pull request?

We will work on this pull request indirectly.
We might merge it during the development or pull parts of it into the new version.
This means that during the development phase, we aim to tackle it.
Maybe in a different way like it is currently handled.
Please understand that this will take a while because we are running this in our spare time.

Final words

Thanks for using and contributing to this library.
If there is anything else you would like to tell us, let us know!

@github-actions github-actions bot added the conflicts Indicates merge conflicts label Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change conflicts Indicates merge conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants