Thank you, @pankona.

Can you please take a look at #966 and #963 for me?
I obviously didn't fix the original problem correctly since it is still an issue.
Yet the original code used this (see https://github.com/google/go-github/blame/fbd7fb9c7f3d27aa691e8421690e00be916f8d8d/github/search.go):

import (
	qs "github.com/google/go-querystring/query"
)
...
	params, err := qs.Values(opt)
	if err != nil {
		return nil, err
	}
	params.Set("q", parameters.Query)
	if parameters.RepositoryID != nil {
		params.Set("repository_id", strconv.FormatInt(*parameters.RepositoryID, 10))
	}
	u := fmt.Sprintf("search/%s?%s", searchType, params.Encode())

and I'm wondering if the correct fix is to make the changes in the upstream go-querystring/query package instead?

Thoughts?

@gmlewis

I've checked #966 and #963 .
I've pushed a new commit 6bc0bca . I think go-querystring has no problem. Could you please check current patch is as you expected?

Thank you, @pankona! I'll have to take time and look at this tonight... gotta run... but in a nutshell, was #966 just broken? I'm curious how this current PR differs from the original, but don't have time right now to look... I just know there was a problem in #963 that needed fixing and hope that this addresses that issue as well.

@gmlewis

I've checked snippet on #963 (as follows)

rs, _, err := client.Search.Repositories(context.Background(),
		"language:golang+stars:>=200+pushed:>=2018-01-01",
		&github.SearchOptions{Sort: "stars", Order: "desc",
				ListOptions: github.ListOptions{Page: 2, PerPage: 100}})

This query string includes "+" (instead of white space). I think this is NOT acceptable query string.

• "language:golang+stars:>=200+pushed:>=2018-01-01" doesn't work with GitHub's search input form
• According to GitHub's search API documentation, there're some examples that include "+" instead of white space, I think the white spaces are already escaped.
• I think the query string written above should be "language:golang stars:>=200 pushed:>=2018-01-01"

This query string includes "+" (instead of white space). I think this is NOT acceptable query string.

...although it worked when using curl... does your recommended change also work against the GitHub v3 API? If so, then I'm happy to incorporate your solution.

But are you saying here that the original code was correct?

@gmlewis

But are you saying here that the original code was correct?

Yes. I think original code was correct.

In my understanding, escaping of query string is responsibility of go-github itself, but not of library user. Thus a query string that includes "+" instead of white space may return unexpected result.

curl

• Here's curl command using "+" instead of white spaces (got OK)

curl -X GET "https://api.github.com/search/repositories?order=desc&page=2&per_page=100&q=language%3Agolang+stars%3A%3E%3D200+pushed%3A%3E%3D2018-01-01&sort=stars"

• Here's curl command using white spaces directly (got error)

curl -X GET "https://api.github.com/search/repositories?order=desc&page=2&per_page=100&q=language%3Agolang stars%3A%3E%3D200 pushed%3A%3E%3D2018-01-01&sort=stars"

In case of using curl, escaping of query string is responsibility of user. Thus former case goes okay.

Thank you for the analysis, @pankona! It is greatly appreciated!

@gauntface - do you agree?

TL;DR: #966 was the wrong fix for #963 - the query in #963 was malformed. This PR goes back to the original implementation.

Ok, my understanding from everything is that this is the correct change.

Forcing users to not do this:

client.Search.Repositories(ctx, "language:golang+stars:>=200+pushed:>=2018-01-01", ...)

And instead do this:

client.Search.Repositories(ctx, "language:golang stars:>=200 pushed:>=2018-01-01", ...)

I like this approach just from the perspective of being close to the input you would provide to the Github.com search box.

Regarding this PR, @pankona could you add a comment to the search() method calling out the example input of " " vs "+" and highlight that we expect " ".

@gauntface
Thank you for checking!

comment

okay, I will.

@gmlewis

understood, I will!

@gmlewis

At b94a2b5, I introduced c++ in unit tests to make escape result more clear. How about it?

Perfect, @pankona ! Thank you!

Awaiting @gauntface LGTM before merging.

Gently ping to @gauntface :)

Thank you @gauntface for your confirmation!

@gmlewis

If there's no concern for merging, could you please proceed?

Thank you, @gauntface!
Merging.