Skip to content

Make search API of github library work again #321

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

Merged
merged 6 commits into from
Feb 1, 2018
Merged

Make search API of github library work again #321

merged 6 commits into from
Feb 1, 2018

Conversation

tkvogt
Copy link
Contributor

@tkvogt tkvogt commented Jan 31, 2018

Using recent patch to http-types to fix github issue #262.

@snoyberg
Copy link
Owner

snoyberg commented Feb 1, 2018

I don't follow how the added function address the title in this PR. Also, can you include a change of entry and bump the version in the cabal file?

@tkvogt
Copy link
Contributor Author

tkvogt commented Feb 1, 2018

I forgot to mention this issue, where it is explained.
Github library uses http-client which uses http-types. http-types now allows to explicitly say which part of the query to encode. If you merge this patch, it is possible to use '+' or ':' in a search keyword, or what is more important to prevent '+' and ':' to be URL encoded and destroy a query like this:

?q=a%2Bcreated%3A2012-12-07..2012-12-17

@snoyberg
Copy link
Owner

snoyberg commented Feb 1, 2018

OK, got it, this makes sense now. There's only one problem still: Stackage has been unable to upgrade to the newer version of http-types because so many packages are incompatible with it. I think it would make sense to make this new function's presence conditional on the version of http-types, instead of adding a lower bound.

@snoyberg
Copy link
Owner

snoyberg commented Feb 1, 2018

This removes the lower bound, but doesn't make inclusion of the function conditional on the http-types version, meaning that this will simply break for older versions of http-types (Travis and AppVeyor will likely demonstrate this).

@snoyberg snoyberg merged commit 0ab0a4e into snoyberg:master Feb 1, 2018
@snoyberg
Copy link
Owner

snoyberg commented Feb 1, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document search string format
2 participants