SearchIssuesRequest with State = ItemState.All

[hahmed]

https://github.com/octokit/octokit.net/blob/master/Octokit/Clients/SearchClient.cs#L52

Searching issues via the search api does not look like it is working when setting the state parameter to ItemState.All, this only seems to work for ItemState.Open or ItemState.Closed.

e.g. https://github.com/search?q=state%3Aall+repo%3Arails%2Frails&type=Issues

Does not yield any results. I have tried out the code below too:

            var repo = new RepositoryCollection();
            repo.Add("rails", "rails");
            var issueSearch = new SearchIssuesRequest
            {
                 Repos = repo,
                 State = ItemState.All,
                 Created = DateRange.GreaterThan(new DateTime(DateTime.Now.Year, 01, 01).AddYears(-1)),
            };
            var railsIssues = await _githubClient.Search.SearchIssues(issueSearch);

However if I set the State to either ItemState.Open ItemState.Closed` it will work, or if I omit it, it will also work. I think when it is set to ItemState.All we need to not add it to the request parameter.

The api seems to correlate to what I am saying to? https://help.github.com/articles/searching-issues/#search-based-on-whether-an-issue-or-pull-request-is-open

I hope that makes sense. Let me know if I need to explain further.

[hahmed]

I added a working example of the search api in my sample:

https://github.com/hahmed/Sprint/blob/master/Sprint/Controllers/HomeController.cs#L118

[shiftkey]

@hahmed can we capture this as a failing integration test in the repo?

[hahmed]

@shiftkey yep sure.

So I just need to write a failing test for:

https://github.com/octokit/octokit.net/blob/master/Octokit.Tests.Integration/Clients/SearchClientTests.cs

For example the below would be a valid failing integration test?

    [Fact]
    public async Task SearchForAllIssuesFails()
    {
        var request = new SearchIssuesRequest("phone");
        request.Repos.Add("caliburn-micro", "caliburn.micro");
        request.State = ItemState.All;

        var issues = await _gitHubClient.Search.SearchIssues(request);

        Assert.NotEmpty(issues.Items);
    }

[shiftkey]

@hahmed that seems good enough for me - and if you could mark the test as [Fact (Skip="see https://github.com/octokit/octokit.net/issues/1082 for investigating this failing test")] this will help link back to this discussion

[hahmed]

@shiftkey done.

#1083

[prayankmathur]

@shiftkey To do the above I removed "All" from the ItemState enum.

And regarding the failing integration tests, by simply removing the query for ItemState.All .
https://github.com/prayankmathur/octokit.net/blob/master/Octokit.Tests.Integration/Clients/SearchClientTests.cs#L94

Also from the IssueClientTests.cs https://github.com/prayankmathur/octokit.net/blob/master/Octokit.Tests.Integration/Clients/IssuesClientTests.cs#L162

and the build succeeded.
https://ci.appveyor.com/project/prayankmathur/octokit-net

Does this do the needful ?

[M-Zuber]

@prayankmathur How then would someone search for all issues?

[prayankmathur]

@M-Zuber
"All" isn't supported by the github API.
It is only a filter.
To search for it you need not have to mention any ItemState.

[M-Zuber]

Sounds great.
Open a Pull Request?

[ryangribble]

Have to be a bit careful here because ItemState enumeration is used in many places (Milestones, Events, Pull Requests, Issues, and more) and sometimes all is a vaild option and sometimes not?

Eg SearchIssueRequest (only open and closed are allowed) vs IssueRequest (where open closed and all) are allowed

https://developer.github.com/v3/search/#search-issues
https://developer.github.com/v3/issues/#list-issues

So I dont think removing All from the enum is good. Perhaps a new enum is needed and those places where only open/closed state is allowed, are changed to the new enum?

[prayankmathur]

@ryangribble @M-Zuber @shiftkey
What should be done here ?

[M-Zuber]

@ryangribble Thank you for pointing that out.

@prayankmathur My suggestion would be to

• Create a new enum ExpandedItemState (or the like, @ryangribble might have a better name)
• Find all the places the ItemState enumeration is used, and check the corresponding api docs
  • Where All is a valid option, use the new enum.

[prayankmathur]

@M-Zuber
Will get to it.

[shiftkey]

So I dont think removing All from the enum is good.

Also, we should be marking things as [Obsolete] before removing them. Because we're nice like that.

[shiftkey]

I like where this is heading. If it were me, I'd probably introduce a new enum in the search API instead of sharing the ItemState enum as it doesn't support All.

This feels like we're going to need to introduce a breaking change here as part of the new enum. But that's fine. Just a heads up.

[prayankmathur]

@shiftkey
I will look into as to where all the changes are to made and then get back here.

[M-Zuber]

I like where this is heading. If it were me, I'd probably introduce a new enum in the search API instead of sharing the ItemState enum as it doesn't support All.

So the opposite of what I suggested, which does make more sense as it is much less of a change :)

[hahmed]

Or alternatively on the line: https://github.com/octokit/octokit.net/blob/master/Octokit/Models/Request/SearchIssuesRequest.cs#L248

We can simply change the code from:

 if (State.HasValue)
            {
                parameters.Add(string.Format(CultureInfo.InvariantCulture, "state:{0}", State.Value.ToParameter()));
            }

to

 if (State.HasValue && State != ItemState.All)
            {
                parameters.Add(string.Format(CultureInfo.InvariantCulture, "state:{0}", State.Value.ToParameter()));
            }

or something along those lines... just a far simpler change IMHO. Maybe add a link to the api/issue to explain if needs be.

[prayankmathur]

@hahmed that won't do much good because then we need to check if "All" state exists for that particular parameter or not and there are a lot of parameters.

[ryangribble]

yeah, its preferable to not show an option (ie an All entry in Enum) to a user when it doesn't actually exist in the GitHub API like that. I'd make a new enum called SearchItemState or ItemStateSimple or something like that, and use that on the Search functions (and anywhere else that might be limited to only open/closed). I'd also update the XmlDoc comments on the ItemStateSimple? State parameter of SearchIssueRequest to note that leaving it unspecified will enact the GitHub default

[hahmed]

@ryangribble State.All does exist... just implicitly e.g.

https://github.com/search?p=3&q=repo%3Arails%2Frails&ref=searchresults&type=Issues&utf8=%E2%9C%93

The result above returns both open and closed issues. Hence my suggestion - remove the param from being added on the condition State != ItemState.All Regardless of which way this is implemented - the search issues needs to give back:

1. open issues
2. closed issues
3. open and closed issues

If there are any other areas of the code where ItemState.All is relevant, append it to the parameters. If not - do not add it, we just need to ensure the expected results match the GitHub api.

-EDIT-

Unless passing in null refers to search both open and closed issues...? Then yeah - we should create a new enum as suggested.

[prayankmathur]

@ryangribble @shiftkey @hahmed
I have created the new enum by the name ItemStateFilter
And read the docs and made changes everywhere as said by @M-Zuber
Also made the corresponding changes in the unit as well as integration tests.
The build was successful link
My forked repository is here

[M-Zuber]

Could you please open a PR?

If you are not sure how that works, may I suggest watching this video? (the whole series actually is quite nice)

[prayankmathur]

@M-Zuber
👍
just opened the PR .
And thanks for the video link. 😄

[ryangribble]

Thanks for the PR! Ive just made a comment on the PR and basically said that after seeing the proposed changes, i've backflipped on what was said above!

Please check #1140 for my comments and let's continue the discussion there 😀