Skip to content
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

Partial escape in query string to make search work again #321

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tkvogt
Copy link

@tkvogt tkvogt commented Apr 9, 2018

fixes #262
http-types and http-client had to be extended for partial escape. These versions are now in stackage quite a while. I built the structure that I assumed you had in mind (SearchRepoMod), but it is very tedious and in my opinion not worth the trouble. So I used two styles in GitHub.Endpoints.Search

  • Using SearchRepoMod in: searchRepos
  • Constructing the query directly: searchCode' [("q", [QE "language", QN ":", QE "haskell"]), ..

Copy link
Contributor

@phadej phadej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, but there are some nitpicks inline.

Orphan instance is a blocker.

@@ -39,6 +39,7 @@ module GitHub.Endpoints.Organizations.Teams (
import GitHub.Data
import GitHub.Internal.Prelude
import GitHub.Request
import qualified Network.HTTP.Types as W
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use stylish-haskell, to make import pretty put qualified imports into own section:

import Github.Request
import Prelude ()

import qualified Network.HTTP.Types as W

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -56,4 +57,5 @@ nestedTree = nestedTree' Nothing
-- See <https://developer.github.com/v3/git/trees/#get-a-tree-recursively>
nestedTreeR :: Name Owner -> Name Repo -> Name Tree -> Request k Tree
nestedTreeR user repo sha =
query ["repos", toPathPart user, toPathPart repo, "git", "trees", toPathPart sha] [("recursive", Just "1")]
query ["repos", toPathPart user, toPathPart repo, "git", "trees", toPathPart sha]
[("recursive", [W.QE "1"])]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 space indentation (tabular formatting doesn't apply here).

@@ -241,6 +242,14 @@ instance Hashable (SimpleRequest k a) where
`hashWithSalt` ps
`hashWithSalt` body

instance Hashable W.EscapeItem where
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

orphan instance :/.

Could you make a PR to http-types so the instance is there. Alternatively inline code where instance is needed?

I won't accept PR with orphan instance.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have done this. But now it have wrapped the type in a newtype, because I don't want to go through the whole process of having the right http-types version in stackage.

-- "q=a+repo:phadej/github&per_page=100"
-- has to be written as
-- > searchIssues [("q", [QE "a", QN "+", QE "repo", QN ":", QE "phadej", QN "/", QE "github"]),
-- ("per_page", [QE "100"])]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite sure this looks ugly when rendered. > have to prefix all code lines.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the next version haddock is tested

@andreasabel
Copy link
Member

@tkvogt : Thanks for this PR!
It's been a while; do you want to finish this, fixing the conflicts and what remained to be done?

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please:

  • fix conflicts
  • make sure AllHaskellRepos runs

@tkvogt
Copy link
Author

tkvogt commented Apr 19, 2022

This is so long ago. I just remeber that it worked and was good enough for me. It turned into some kind of beauty contest, which I had no time for.

@andreasabel
Copy link
Member

It turned into some kind of beauty contest, which I had no time for.

I understand. Sorry for that.

My priorities are rather:

  1. the functionality is faithful to the GitHub API
  2. there are tests or demos so the functionality can be verified

I didn't look closely at 1., but I noticed that 2. is satisfied (AllHaskellRepos).

@tkvogt
Copy link
Author

tkvogt commented Apr 19, 2022

If you use stack, you can reference this pull request and try it like

- git: https://github.com/tkvogt/github.git
  commit: 0e157e2e9c2f75e44e34bc0ffe67f2155326eba1

In my memory it worked.
Of course If you somehow finish it, you are the hero.

@andreasabel
Copy link
Member

Of course If you somehow finish it, you are the hero.

Got enough on my plate. I suppose this has to wait for someone with a genuine interest in this functionality to pick it up...

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
3 participants