Refactor SearchIssuesRequest to match the API #290

hnrkndrssn · 2014-01-02T08:21:14Z

Update SearchIssuesRequest to match current API
Re-Add Sort property
Add tests
Unskip tests awaiting PR Fix date format used in DateRange #293
Refactor to use BaseSearchRequest (Base class for searching GitHub #301)

Fixes #274

hnrkndrssn · 2014-01-07T23:56:25Z

This is ready for review. Just waiting for #293 so I can remove Skip on a few tests.

cc @shiftkey

shiftkey · 2014-01-08T03:31:43Z

@alfhenrik oops, I meant to merge that in last night. Done!

hnrkndrssn · 2014-01-08T03:47:32Z

Sweeet, I'll update the skipped tests tonight 😀

hnrkndrssn · 2014-01-08T07:51:51Z

Dun, and ready for review ✨

shiftkey · 2014-01-08T11:38:49Z

Octokit.Tests/Clients/SearchClientTests.cs

+
+ client.SearchIssues(request);
+
+ connection.Received().GetAll<Issue>(Arg.Is<Uri>(u => u.ToString() == "search/issues"), Arg.Is<Dictionary<string, string>>(d => d["q"].StartsWith("pub")));


Trim these lines at 80 char or thereabouts...

hnrkndrssn · 2014-01-08T12:21:40Z

Didn't quite get all lines down to 80'ish...but should be better now.

...even an extra one in the Search Code tests...ooops..:grin:

shiftkey · 2014-01-09T01:12:44Z

Octokit.Tests/Clients/SearchClientTests.cs

+
+ connection.Received().GetAll<Issue>(
+ Arg.Is<Uri>(u => u.ToString() == "search/issues"), 
+ Arg.Is<Dictionary<string, string>>(d => d["q"].StartsWith("pub")));


Any particular reason why you chose to use StartsWith and Contains rather than just hard-coding the strings?

I think tests should be tolerable, but given the possible set of query terms is large, is it worth being more terse with these checks?

EDIT: terse, not succinct. So doing d["q"] == "pub" for example in the test above.

I'm sure there was a valid reason at the time why I went with StartsWith and Contains, but I'll take a look and see if we can be more terse as suggested.

shiftkey · 2014-01-09T01:22:40Z

Didn't quite get all lines down to 80'ish...but should be better now.

That's why I said "or thereabouts" 😀 - much more easier to read now.

The changes look great, just curious if we can make the tests a bit more robust...

hnrkndrssn · 2014-01-09T08:15:17Z

While testing this, I found that the query should be formatted like q=searchterm+params but the code currently does q=searchterm params, I have updated the code to:

var mergedParameters = MergeParameters();
d.Add("q", Term + (mergedParameters.IsNotBlank() ? "+" + mergedParameters : ""));

I just want to check if that is an acceptable solution.

shiftkey · 2014-01-09T08:31:38Z

cc @hahmed

hahmed · 2014-01-09T09:53:24Z

👍 with that suggestion

hnrkndrssn · 2014-01-09T10:01:55Z

Alright. I'll admit it, there was probably no good reason for using StartsWith and Contains instead of hard coded strings 😞 ...have refactored to use hard coded strings instead.

shiftkey · 2014-01-09T10:08:50Z

Alright. I'll admit it, there was probably no good reason for using StartsWith and Contains instead of hard coded strings

Don't sweat it. We're only human.

hahmed · 2014-01-09T10:10:50Z

Octokit/Models/Request/SearchIssuesRequest.cs

@@ -25,14 +27,13 @@ public SearchIssuesRequest(string term)
 /// The search terms. This can be any combination of the supported repository search parameters:
 /// http://developer.github.com/v3/search/#search-code


http://developer.github.com/v3/search/#search-issues?

hahmed · 2014-01-09T10:22:29Z

The changes look great, just curious if we can make the tests a bit more robust...

Why not test out each search request? Every qualifier too? e.g. In, Author etc.

            [Fact]
            public void TestingQualifiers()
            {
                var request = new SearchIssuesRequest("something");
                Assert.Equal("something", request.Term);
            }

That way we can assert each property does what it should? Quite a lot of tests to add mind you...

hnrkndrssn · 2014-01-09T10:25:30Z

That's what d["q"] does, as it checks each property on the request and creates the query string from that.

shiftkey · 2014-01-09T10:31:43Z

@hahmed I'd prefer to test that we're sending the correct query format to the GitHub API (harder), rather than that the parameters are populated as expected (easier).

hahmed · 2014-01-09T11:01:13Z

Just a suggestion:

One thing that I was thinking about whilst I was implementing the search api was ensure everything get's appending correctly to the term.

I wanted to expose a TermAndQualifiers property so we can better test the output

public
{
   get { return Term + (mergedParameters.IsNotBlank() ? "+" + mergedParameters : ""); }
}

Then we can test what the final string should actually look like, for example github search for a user called mike:

Assert.Equals("mike+type:user", request.TermAndQualifiers);

shiftkey · 2014-01-09T12:44:14Z

I'm really interested in the external behaviour of these actions - i.e. what the client sends to the server.

Testing properties in this fashion is just testing a layer away from the important bits.

hnrkndrssn · 2014-01-09T21:52:14Z

Octokit.Tests/Clients/SearchClientTests.cs

+ Arg.Is<Uri>(u => u.ToString() == "search/issues"), 
+ Arg.Is<Dictionary<string, string>>(d => 
+ d["sort"] == "Updated" &&
+ d["order"] == "Descending"));


Something I found last night redoing these tests is that the order parameter is coming back as Descending, but after having a look at the API docs here http://developer.github.com/v3/search/#parameters-2 I realised the order parameter is either asc or desc.

The SortDirection enum (https://github.com/octokit/octokit.net/blob/master/Octokit/Models/Request/IssueRequest.cs#L91) values has a Parameter(Value="") attribute applied to it, so should that automagically ✨ convert Descending to desc or is there something that needs to be done.

Or, will the API accept Descending as the value of the order parameter?

cc @shiftkey

Calling .ToString() on an enum will ignore whatever [Parameter] you have set, so this definitely feels like a bug.

Or, will the API accept Descending as the value of the order parameter?

No, it doesn't even return valid search results

shiftkey · 2014-01-14T02:34:51Z

#301 was merged in, so you should be unblocked now

hnrkndrssn · 2014-01-14T02:37:01Z

👍

hnrkndrssn · 2014-01-14T08:14:36Z

Ready for another round of review now after move to use BaseSearchRequest

cc @shiftkey

shiftkey · 2014-01-18T00:35:07Z

Fix them projects! :)

shiftkey · 2014-01-18T00:45:39Z

Octokit/Models/Request/SearchIssuesRequest.cs

+ }
+
+ /// <summary>
+ /// https://help.github.com/articles/searching-issues#language


Could we move the URLs into a <remarks> element and instead make a friendly sentence for each of these?

To extract the Value field from the Parameter Attribute

instead of hard coded strings

Move URI to <remarks> field

shiftkey · 2014-01-18T17:38:47Z

Really like how this turned out - thanks for all the hard work on this!

Refactor SearchIssuesRequest to match the API

hnrkndrssn · 2014-01-18T19:34:46Z

Awesome, we got there in the end :) Thanks for all the feedback!

shiftkey mentioned this pull request Jan 6, 2014

Search/Users Api #289

Merged

2 tasks

shiftkey reviewed Jan 8, 2014
View reviewed changes

shiftkey reviewed Jan 9, 2014
View reviewed changes

hahmed reviewed Jan 9, 2014
View reviewed changes

hnrkndrssn reviewed Jan 9, 2014
View reviewed changes

This was referenced Jan 10, 2014

Search base class hahmed/octokit.net#1

Closed

Base class for searching GitHub #301

Merged

shiftkey reviewed Jan 18, 2014
View reviewed changes

hnrkndrssn added 16 commits January 18, 2014 18:37

Refactor SearchIssuesRequest to match the API

6cf7f96

Fix spelling mistake

13720b9

Add sort order to parameters

ed20952

Add unit tests

d173e18

Remove skip from unit tests

48d931d

Keeping line lengths to about 80

66b1dd3

Fix how params are added to querystring

535d12e

Change tests to use hard coded query strings

3758ccd

Hard code result string in some more tests

d8f810a

Fix incorrect URL

64a7f5a

Add links to search doco for issues request props

104d3e9

Add ToParamater extension method on Enums

17d9e53

To extract the Value field from the Parameter Attribute

Refactor to use BaseSearchRequest

5423cfc

Call ToParameter extension in base class

ffa4bab

instead of hard coded strings

Add EnumExtension class to *.mono projects

1d3d5ca

Rearrange comments to include a description

2919b1e

Move URI to <remarks> field

shiftkey added a commit that referenced this pull request Jan 18, 2014

Merge pull request #290 from alfhenrik/search-issues-req

ad987cd

Refactor SearchIssuesRequest to match the API

shiftkey merged commit ad987cd into octokit:master Jan 18, 2014

hnrkndrssn deleted the search-issues-req branch January 18, 2014 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor SearchIssuesRequest to match the API #290

Refactor SearchIssuesRequest to match the API #290

hnrkndrssn commented Jan 2, 2014

hnrkndrssn commented Jan 7, 2014

shiftkey commented Jan 8, 2014

hnrkndrssn commented Jan 8, 2014

hnrkndrssn commented Jan 8, 2014

shiftkey Jan 8, 2014

hnrkndrssn commented Jan 8, 2014

shiftkey Jan 9, 2014

hnrkndrssn Jan 9, 2014

shiftkey commented Jan 9, 2014

hnrkndrssn commented Jan 9, 2014

shiftkey commented Jan 9, 2014

hahmed commented Jan 9, 2014

hnrkndrssn commented Jan 9, 2014

shiftkey commented Jan 9, 2014

hahmed Jan 9, 2014

hahmed commented Jan 9, 2014

hnrkndrssn commented Jan 9, 2014

shiftkey commented Jan 9, 2014

hahmed commented Jan 9, 2014

shiftkey commented Jan 9, 2014

hnrkndrssn Jan 9, 2014

shiftkey Jan 10, 2014

shiftkey commented Jan 14, 2014

hnrkndrssn commented Jan 14, 2014

hnrkndrssn commented Jan 14, 2014

shiftkey commented Jan 18, 2014

shiftkey Jan 18, 2014

hnrkndrssn Jan 18, 2014

shiftkey commented Jan 18, 2014

hnrkndrssn commented Jan 18, 2014


		client.SearchIssues(request);

		connection.Received().GetAll<Issue>(Arg.Is<Uri>(u => u.ToString() == "search/issues"), Arg.Is<Dictionary<string, string>>(d => d["q"].StartsWith("pub")));

		@@ -25,14 +27,13 @@ public SearchIssuesRequest(string term)
		/// The search terms. This can be any combination of the supported repository search parameters:
		/// http://developer.github.com/v3/search/#search-code

Refactor SearchIssuesRequest to match the API #290

Refactor SearchIssuesRequest to match the API #290

Conversation

hnrkndrssn commented Jan 2, 2014

hnrkndrssn commented Jan 7, 2014

shiftkey commented Jan 8, 2014

hnrkndrssn commented Jan 8, 2014

hnrkndrssn commented Jan 8, 2014

Choose a reason for hiding this comment

hnrkndrssn commented Jan 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiftkey commented Jan 9, 2014

hnrkndrssn commented Jan 9, 2014

shiftkey commented Jan 9, 2014

hahmed commented Jan 9, 2014

hnrkndrssn commented Jan 9, 2014

shiftkey commented Jan 9, 2014

Choose a reason for hiding this comment

hahmed commented Jan 9, 2014

hnrkndrssn commented Jan 9, 2014

shiftkey commented Jan 9, 2014

hahmed commented Jan 9, 2014

shiftkey commented Jan 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiftkey commented Jan 14, 2014

hnrkndrssn commented Jan 14, 2014

hnrkndrssn commented Jan 14, 2014

shiftkey commented Jan 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiftkey commented Jan 18, 2014

hnrkndrssn commented Jan 18, 2014