-
Notifications
You must be signed in to change notification settings - Fork 49
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
GitHub example #162
GitHub example #162
Conversation
Codecov Report
@@ Coverage Diff @@
## master #162 +/- ##
=======================================
Coverage 96.57% 96.57%
=======================================
Files 6 6
Lines 175 175
Branches 5 5
=======================================
Hits 169 169
Misses 6 6 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@purrgrammer the code looks good but I'd try to keep the user attention on the important things. In order to do that, and based on my comments:
- I would move the
private def
fetchDataRecursively
with its nestedhasNext
,getNextLink
andgetNext
def
s to the end of the file - I would move
fetchOrgRepos
,fetchLanguages
andfetchContributors
inside of their corresponding datasources
In that way, you'll see in the example:
- The three DataSources
- The two tests
- And then, only if I'm interested, the implementation details of the GitHub calls
Does it make sense?
|
||
val REL_NEXT = "rel=\"next\"".r | ||
|
||
def hasNext[F[_]: ConcurrentEffect](res: Response[F]): Boolean = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is used only inside private
functions, can we define it as private
too?
}) | ||
} | ||
|
||
def getNext[F[_]: ConcurrentEffect](res: Response[F]): F[Uri] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
REL_NEXT.findFirstIn(h.value).isDefined | ||
}) | ||
|
||
def getNextLink[F[_]: ConcurrentEffect](raw: String): F[String] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
|
||
implicit val repoD: Decoder[Repo] = deriveDecoder | ||
|
||
private def fetchOrgRepos[F[_]](c: Client[F], req: Request[F])( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These three functions follow the same pattern, I'm guessing if we could just write one generic function and then move the hasNext
, getNextLink
and getNext
inside of it:
private def fetchDataRecursively[F[_]: ConcurrentEffect, A](c: Client[F], req: Request[F])(decode: Response[F] => F[List[A]])(
implicit C: ConcurrentEffect[F]
): F[List[A]] = {
val REL_NEXT = "rel=\"next\"".r
def hasNext(res: Response[F]): Boolean =
res.headers
.get(CaseInsensitiveString("Link"))
.fold(false)({ h =>
REL_NEXT.findFirstIn(h.value).isDefined
})
def getNextLink(raw: String): F[String] = {
REL_NEXT
.findFirstMatchIn(raw)
.fold(
Sync[F].raiseError[String](new Exception("Couldn't find next link"))
)(m => {
Sync[F].pure(m.before.toString.split(",").last.trim.dropWhile(_ == '<').takeWhile(_ != '>'))
})
}
def getNext(res: Response[F]): F[Uri] =
res.headers
.get(CaseInsensitiveString("Link"))
.fold(Sync[F].raiseError[Uri](new Exception("next not found")))(
raw => getNextLink(raw.value).map(Uri.unsafeFromString(_))
)
for {
result <- c.fetch[List[A]](req) {
case Status.Ok(res) =>
if (hasNext(res)) {
for {
data <- decode(res)
nxt <- getNext(res)
newReq = req.withUri(nxt)
moreData <- fetchDataRecursively(c, newReq)(decode)
} yield data ++ moreData
} else
res.as[List[A]]
case res =>
C.raiseError(new Exception(res.body.toString))
}
} yield result
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we could have:
private def fetchOrgRepos[F[_]](c: Client[F], req: Request[F])(
implicit C: ConcurrentEffect[F]
): F[List[Repo]] = {
implicit val reposED: EntityDecoder[F, List[Repo]] = jsonOf
fetchDataRecursively[F, Repo](c, req)(_.as[List[Repo]])
}
private def fetchLanguages[F[_]](c: Client[F], req: Request[F])(
implicit C: ConcurrentEffect[F]
): F[List[Language]] = {
implicit val objED: EntityDecoder[F, JsonObject] = jsonOf
fetchDataRecursively[F, Language](c, req)(_.as[JsonObject].map(_.toList.map(_._1)))
}
private def fetchContributors[F[_]](c: Client[F], req: Request[F])(
implicit C: ConcurrentEffect[F]
): F[List[Contributor]] = {
implicit val objED: EntityDecoder[F, List[Contributor]] = jsonOf
fetchDataRecursively[F, Contributor](c, req)(_.as[List[Contributor]])
}
|
||
import fetch.{DataSource, Env, Fetch} | ||
|
||
class GithubExample extends WordSpec with Matchers { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to use http://47deg.github.io/github4s/ instead of the custom Http client and unsafe building of Uris?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK it doesn't support the call to get languages but is something I can add to github4s
. Good point.
9025e45
to
eea84b3
Compare
GitHub example
A small example of using Fetch to request data from Github. In this case we are doing the same data fetches that org does. The main example is:
Fetch executes the above in two rounds of execution, making as much requests in parallel as possible.
The second example simply fetches multiple repositories in parallel.
Under the hood we're also following Github pagination links.