FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Partial escape in query string to make search work again by tkvogt · Pull Request #321 · haskell-github/github · GitHub

/ github Public

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

Open
tkvogt wants to merge 4 commits into
haskell-github:masterfrom
tkvogt:master
ConversationCommits (4)ChecksFiles changed
Open

Partial escape in query string to make search work again#321
tkvogt wants to merge 4 commits into
haskell-github:masterfrom
tkvogt:master

Conversation

tkvogt commented Apr 9, 2018

Copy link
Copy Markdown

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"]), ..

phadej suggested changes Apr 9, 2018
View reviewed changes

phadej left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks great, but there are some nitpicks inline.

Orphan instance is a blocker.

Comment thread src/GitHub/Endpoints/Organizations/Teams.hs Outdated
import GitHub.Data
import GitHub.Internal.Prelude
import GitHub.Request
import qualified Network.HTTP.Types as W

phadej Apr 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tkvogt Apr 9, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ok

Comment thread src/GitHub/Endpoints/GitData/Trees.hs Outdated
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"])]

phadej Apr 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread src/GitHub/Data/Request.hs Outdated
`hashWithSalt` ps
`hashWithSalt` body

instance Hashable W.EscapeItem where

phadej Apr 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

tkvogt Apr 9, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread src/GitHub/Endpoints/Search.hs Outdated
-- "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"])]

phadej Apr 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tkvogt Apr 9, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In the next version haddock is tested

andreasabel commented Apr 19, 2022

Copy link
Copy Markdown
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?

andreasabel requested changes Apr 19, 2022
View reviewed changes

andreasabel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please:

  • fix conflicts
  • make sure AllHaskellRepos runs

tkvogt commented Apr 19, 2022

Copy link
Copy Markdown
Author

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 commented Apr 19, 2022

Copy link
Copy Markdown
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 commented Apr 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

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 commented May 2, 2022

Copy link
Copy Markdown
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...

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

andreasabel andreasabel requested changes
+1 more reviewer
phadej phadej requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Document search string format

3 participants

Back | FazBrowse Home | New Git URL