Implement additional `SearchIssuesRequest` options #1228

ryangribble · 2016-03-29T13:56:21Z

Status

Implemented new options
Unit tests for the request parameters for all new (and existing types)
Flesh out some integration tests to use some of the new options
Get feedback on naming of No property and IssueNoMetadataQualifier enum
Get feedback on implementation of exclusion parameters
Guidance from @haacked +/- @shiftkey on whether to include additional fields other than label in the exclusion support

Details

Fixes #1227
https://developer.github.com/v3/search/#search-issues
https://help.github.com/articles/searching-issues

Added new options to SearchIssuesRequest

Searching by team
Searching by "not" label syntax -label
Searching on missing metadata no
The "human" styled is query
- (although state and type cover most of the is options, the merged/unmerged have no alternative)
Searching by commit status
Searching by branch names base or head
Searching on closed date range

Testing

Added unit tests for each parameter on this class to verify that when not set, the parameter is not in the query string, and when set the parameter is set as expected

Added some integration tests that excercise some of the new options

Discussion

I'd appreciate feedback on the implementation for "excluding" labels. In the API docs, this is done by using notation -label:labelname as opposed to label:labelname to include a label. At first I thought we could just implement this by having any label prepended with a hyphen be deemed to be an "exclusion" label, however it turns out that it's actually valid to have labels that start with a hyphen. So I implemented this as a separate list property SearchIssuesRequest.NotLabels

~~It feels a bit ugly so if someone has a better way, let me know~~

Update: Now renamed to SearchIssuesRequest.ExcludeLabels
See #1228 (comment) for how the "exclusion" field implementation is shaping up...

Similarly, the "missing metadata" search option, ive called the class property No to match the query property in API docs no, and gone with IssueNoMetadataQualifier as the enum name. Open to better suggestions...

M-Zuber · 2016-03-29T14:18:16Z

A few things off the top of my head:

Have a test that uses a label starting with a hyphen and exclude it
- Codifies the reasoning behind having that separate list
Can a username also start with a -?
- If so, excluding issues by a given user would also necessitate a similar construct

Edit:
Real bran-fart this time: If a number of filterable items can start with a - (meaning to actually filter you'd need to do a --), maybe make a .Not property that contains a nested SearchIssuesRequest? (I know crazy idea but you only live once or something)

ryangribble · 2016-03-29T14:45:38Z

Thanks for the feedback 👍

Unless Im missing something, I dont see anything in the API docs about being able to "negate" any of the other filter types though...

The only thing mentioned is labels (-label:labelname vs label:labelname) and keywords (specifying NOT fish instead of fish as the search term).

Have a test that uses a label starting with a hyphen and exclude it

With the way it's been implemented the label name doesn't really matter. I have a unit test that checks when 2 labels are specified in NotLabel we get the expected -label:label1 -label:label2 arguments. Also I added an integration test that searches octokit.net repo for "up-for-grabs" labelled issues and a 2nd search for NOT "up-for-grabs" labelled issues, then checks to make sure the lists are unique

M-Zuber · 2016-03-29T14:55:34Z

It works on (almost) anything

Have a test that uses a label starting with a hyphen and exclude it

With the way it's been implemented the label name doesn't really matter. I have a unit test that checks when 2 labels are specified in NotLabel we get the expected -label:label1 -label:label2 arguments. Also I added an integration test that searches octokit.net repo for "up-for-grabs" labelled issues and a 2nd search for NOT "up-for-grabs" labelled issues, then checks to make sure the lists are unique

What I was trying to say was have a test that looks for a label that starts with a - so as to show that it is legal for a label to start with that char, and it will not be treated as negation if such a label exists.
Not showing that NotLabel will exclude it, but that Label/Term will include it

shiftkey · 2016-03-29T18:17:54Z

@M-Zuber be careful, the repository issue search on the website is likely to be a bit different to what's available through the API. Someone asked me a while ago about this and I'm pretty sure -label doesn't work through the API when looking at repository issues, for example.

@ryangribble regarding your question about the properties, an alternative might be ExcludeLabels. Personally, I don't think that users should need to know or care that you need to do a - prefix, so this is a good direction to head.

M-Zuber · 2016-03-29T20:47:05Z

:embarrassed: guess I just assumed the api was being dogfooded.
as someone who wrote (albeit a simple one) a query language, hiding syntax behind human readable properties is A Very Good Thing™

Update doc comments for all items Organise order of parameters inline with API docs for easier comparisons

…d work now) Fixup doc comments

…o replace IssueTypeQualifier.PR (which is now marked obsolete)

ryangribble · 2016-03-30T12:46:01Z

rebased on latest master
renamed NotLabels to ExcludeLabels
I also tidied up an inconsistency that was irking me. We refer to pull requests as PullRequest pretty much everywhere in octokit.net - except the IssueTypeQualifier.PR enum value. So to maintain consistent naming and terminology, Ive added IssueTypeQualifier.PullRequest to that enum and flagged the PR value as [Obsolete] so it can be removed sometime down the track 😄

guess I just assumed the api was being dogfooded.

It could well be that the API accepts more options than the doco says, but for now Im thinking of just implementing what the doc says (where labels are the only things that can be excluded).

Although Im thinking perhaps for future proofing (If it does turn out that lots more fields can be negated or even the fact that it's likely in the future more fields would be able to be), I could perhaps move to having a new class to hold all the exclusions (and thus refer to them as normal names like Label).

new SearchIssuesRequest("fish")
{
  Type = IssueTypeQualifier.PullRequest,
  Is = IssueIsQualifier.Merged,
  Exclusions = new SearchIssuesExclusions
  {
      Labels = new[] { "label1", "label2" },
      Author = "M-Zuber" // Docs dont say this is supported but this is where more exclude fields could go if/when added
  }
}

Thoughts? Is collecting any exclusions under a sub class, preferable to ending up with multiple properties like ExcludeLabels ExcludeAuthor ExcludeMentions ExcludeTeam and so on?

M-Zuber · 2016-03-30T13:28:42Z

Although Im thinking perhaps for future proofing (If it does turn out that lots more fields can be negated or even the fact that it's likely in the future more fields would be able to be), I could perhaps move to having a new class to hold all the exclusions (and thus refer to them as normal names like Label).

That is pretty much what I was trying to say with this ⬇️, and your example implementation is nicer 😄

Edit:
Real bran-fart this time: If a number of filterable items can start with a - (meaning to actually filter you'd need to do a --), maybe make a .Not property that contains a nested SearchIssuesRequest? (I know crazy idea but you only live once or something)

M-Zuber · 2016-03-30T13:29:23Z

It could well be that the API accepts more options than the doco says, but for now Im thinking of just implementing what the doc says (where labels are the only things that can be excluded).

Sounds good to me, I just wanted to bring up such a possibility

…d all fields that passed tests for properly working exclusions

ryangribble · 2016-03-31T14:30:39Z

OK so ive just pushed up a change that has moved the exclusions fields out into a subclass, as discussed above. So they are now accessed via SearchIssuesRequest.Exclusions

eg

    // Search for PullRequests that dont have "skip-release-notes" label
    var request = new SearchIssuesRequest();
    request.Type = IssueTypeQualifier.PullRequest;
    request.Repos.Add("octokit", "octokit.net");
    request.Exclusions = new SearchIssuesRequestExclusions
    {
        Labels = new[] { "skip-release-notes" }
    };

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

Regarding which fields are supported, I did some poking at the API and determined that it actually does support quite a few fields in "exclusion" syntax, in addition to the mentioned "label" field.

So the following fields have been implemented in the Exceptions parameters:

Author, Assignee, Mentions, Commenter, Involves, State, Labels, Language, Status, Head, Base
These were all tested and work correctly - integration tests are added to assert this.
Although not mentioned in the docs (except Labels), these do seem useful (ie to search for items where Assignee is not me, or shiftkey is not mentioned etc)

The following fields were not implemented for exclusion for listed reasons:

Type,In,No,Is
These failed integration tests, API doesnt support negating these
Created,Updated,Merged,Closed,Comments
These date/range fields already support greater/less than and "between" syntax, so having an exclusion option doesnt really make sense so I didnt attempt to implement/test
Team
I didnt get time to find a suitable repo that has teams being @mentioned in issues, to run integration tests against
User
Not tested.
Does it make sense to do a search for something in all repos EXCEPT those owned by a user or organisation?
Repos
Not tested.
Does it make sense to do a search in all repos EXCEPT some specified?

Thoughts?
Will definitely need some guidance from @haacked and @shiftkey on this
On the one hand it seems prudent to only implement the mentioned label field for exclusion and "dangerous" to implement something that isnt listed in the API docs. But on the other, being able to exclude by author, target branch etc have real life beneficial use cases, and the API clearly does support them (and the integration tests will detect if they ever cease to work)...

M-Zuber · 2016-04-13T15:42:40Z

Created,Updated,Merged,Closed,Comments
These date/range fields already support greater/less than and "between" syntax, so having an exclusion option doesnt really make sense so I didnt attempt to implement/test

I would say to yes implement (assuming support) as some users may find it easier conceptually to use .Exclusions (BTW great job on naming)
Might be open to doing a PR into this PR for those fields.

User
Not tested.
Does it make sense to do a search for something in all repos EXCEPT those owned by a user or organisation?
Repos
Not tested.
Does it make sense to do a search in all repos EXCEPT some specified?

I vote for yes as I have done both in the past.

One example that covers both is looking for something .Net Core related - but not in anything from Microsoft & Microsoft related repositories.

ryangribble · 2016-04-14T02:31:35Z

Cool... i didnt actually even test/probe those options to see whether api accepts excluding them or not... i will look into it since those use cases do sound reasonable

ryangribble · 2016-04-19T10:04:05Z

Hey @shiftkey just a reminder that I'm waiting on some input from your good self before I take any next steps 😀

I know you've been on the road etc, hoping that since you've assigned this to yourself it is somewhere on your list to get to 🔮

shiftkey · 2016-04-19T11:03:54Z

@ryangribble I haven't forgotten, just been digging myself out from under work since I got back to Oz last week and needed a bit of mundane work to take a break. I'll pick this up tomorrow.

shiftkey · 2016-04-20T10:44:19Z

@ryangribble looking over the recent questions:

The following fields were not implemented for exclusion for listed reasons:

Type,In,No,Is
These failed integration tests, API doesnt support negating these

Works for me.

Created,Updated,Merged,Closed,Comments
These date/range fields already support greater/less than and "between" syntax, so having an exclusion option doesnt really make sense so I didnt attempt to implement/test

Yep, I like this too.

Team
I didnt get time to find a suitable repo that has teams being @mentioned in issues, to run integration tests against

This is fine. We'll cross that bridge if we have to.

User
Not tested.
Does it make sense to do a search for something in all repos EXCEPT those owned by a user or organisation?

Happy to kick this can down the road until someone comes looking for it.

Repos
Not tested.
Does it make sense to do a search in all repos EXCEPT some specified?

I guess if you wanted to offload more of the search work onto the API. But again, really edge case. 👢 this for now.

shiftkey · 2016-04-20T10:47:45Z

Octokit.Tests.Integration/Clients/SearchClientTests.cs

@@ -126,4 +126,392 @@ public SearchClientTests()
        Assert.NotEmpty(closedIssues);
        Assert.NotEmpty(openedIssues);
    }
+
+    [Fact]


I think these should all be [IntegrationTest] to ensure they only run when you have an integration test account setup.

Nice spot, thanks

shiftkey · 2016-04-20T11:27:06Z

Changes look good, integration tests are working fine on my end.

I'm happy with the direction, happy to give it another look now that I finally got around to answering your questions...

ryangribble · 2016-04-20T23:41:18Z

Thanks @shiftkey ill tidy up those few things raised and rebase etc

There was also I guess 2 other things I just wanted a specific 👍 from you on:

I also tidied up an inconsistency that was irking me. We refer to pull requests as PullRequest pretty much everywhere in octokit.net - except the IssueTypeQualifier.PR enum value. So to maintain consistent naming and terminology, Ive added IssueTypeQualifier.PullRequest to that enum and flagged the PR value as [Obsolete] so it can be removed sometime down the track 😄

and

Similarly, the "missing metadata" search option, ive called the class property No to match the query property in API docs no, and gone with IssueNoMetadataQualifier as the enum name. Open to better suggestions...

shiftkey · 2016-04-20T23:44:02Z

👍 to PR -> PullRequest - and ❤️ for doing the obsoletion already
👍 for No

shiftkey · 2016-04-23T00:31:49Z

Octokit.Tests.Integration/Clients/SearchClientTests.cs

+    {
+        var allRequest = new SearchIssuesRequest();
+        allRequest.Repos.Add("octokit", "octokit.net");
+        allRequest.Type = IssueTypeQualifier.PR;


💄 rename this to .PullRequest

Lol, nice pickup!

ryangribble · 2016-04-24T21:56:46Z

All feedback addressed, merged latest master and bashed Travis until it finally got a green run!

shiftkey · 2016-04-29T04:19:20Z

@ryangribble +:100: nice work!

ryangribble added the feature label Mar 29, 2016

ryangribble changed the title ~~[WIP] Implement additional SearchIssuesRequest options~~ Implement additional SearchIssuesRequest options Mar 29, 2016

ryangribble added 6 commits March 30, 2016 22:31

Add missing options to SearchIssuesRequest

f93df69

Update doc comments for all items Organise order of parameters inline with API docs for easier comparisons

Add tests for each search parameter

ccd2698

Add integration tests (including unskipping SearchAll test that shoul…

78c7735

…d work now) Fixup doc comments

Fix impacted test

6d83412

Rename NotLabels to ExcludeLabels

a12e528

For consistency, create a new IssueTypeQualifier.PullRequest option t…

4930a69

…o replace IssueTypeQualifier.PR (which is now marked obsolete)

ryangribble force-pushed the search-issues branch from 7dba4f0 to 4930a69 Compare March 30, 2016 12:31

Fix impacted test

9fe9df7

Move exclusions to a new class, tested against the API and implemente…

142ea7a

…d all fields that passed tests for properly working exclusions

Fix missing XmlDoc tag

4c0fdfa

shiftkey self-assigned this Apr 4, 2016

shiftkey reviewed Apr 20, 2016
View reviewed changes

ryangribble added 3 commits April 23, 2016 09:26

Use correct [IntegrationTest] attribute

a62e580

everyone's always hatin' on the Tuple :p

3de5196

Merge remote-tracking branch 'upstream/master' into search-issues

920bef8

shiftkey reviewed Apr 23, 2016
View reviewed changes

Dont use the field i just obsoleted :)

bc5dc88

shiftkey merged commit 7bb751c into octokit:master Apr 29, 2016

ryangribble deleted the search-issues branch August 6, 2016 22:22

nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement additional `SearchIssuesRequest` options #1228

Implement additional `SearchIssuesRequest` options #1228

ryangribble commented Mar 29, 2016 •

edited

Loading

M-Zuber commented Mar 29, 2016

ryangribble commented Mar 29, 2016

M-Zuber commented Mar 29, 2016

shiftkey commented Mar 29, 2016

M-Zuber commented Mar 29, 2016

ryangribble commented Mar 30, 2016

M-Zuber commented Mar 30, 2016

M-Zuber commented Mar 30, 2016

ryangribble commented Mar 31, 2016

M-Zuber commented Apr 13, 2016

ryangribble commented Apr 14, 2016

ryangribble commented Apr 19, 2016

shiftkey commented Apr 19, 2016

shiftkey commented Apr 20, 2016

shiftkey Apr 20, 2016

ryangribble Apr 22, 2016

shiftkey commented Apr 20, 2016

ryangribble commented Apr 20, 2016 •

edited

Loading

shiftkey commented Apr 20, 2016

shiftkey Apr 23, 2016

ryangribble Apr 23, 2016

ryangribble commented Apr 24, 2016

shiftkey commented Apr 29, 2016

Implement additional SearchIssuesRequest options #1228

Implement additional SearchIssuesRequest options #1228

Conversation

ryangribble commented Mar 29, 2016 • edited Loading

Status

Details

Testing

Discussion

M-Zuber commented Mar 29, 2016

ryangribble commented Mar 29, 2016

M-Zuber commented Mar 29, 2016

shiftkey commented Mar 29, 2016

M-Zuber commented Mar 29, 2016

ryangribble commented Mar 30, 2016

M-Zuber commented Mar 30, 2016

M-Zuber commented Mar 30, 2016

ryangribble commented Mar 31, 2016

M-Zuber commented Apr 13, 2016

ryangribble commented Apr 14, 2016

ryangribble commented Apr 19, 2016

shiftkey commented Apr 19, 2016

shiftkey commented Apr 20, 2016

shiftkey Apr 20, 2016

Choose a reason for hiding this comment

ryangribble Apr 22, 2016

Choose a reason for hiding this comment

shiftkey commented Apr 20, 2016

ryangribble commented Apr 20, 2016 • edited Loading

shiftkey commented Apr 20, 2016

shiftkey Apr 23, 2016

Choose a reason for hiding this comment

ryangribble Apr 23, 2016

Choose a reason for hiding this comment

ryangribble commented Apr 24, 2016

shiftkey commented Apr 29, 2016

Implement additional `SearchIssuesRequest` options #1228

Implement additional `SearchIssuesRequest` options #1228

ryangribble commented Mar 29, 2016 •

edited

Loading

ryangribble commented Apr 20, 2016 •

edited

Loading