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

Clarify error handling design #363

Closed
BenFradet opened this issue Mar 9, 2020 · 4 comments
Closed

Clarify error handling design #363

BenFradet opened this issue Mar 9, 2020 · 4 comments

Comments

@BenFradet
Copy link
Contributor

The relationship GHResult / GHResponse could be more useful, e.g.

GHResponse[A](result: Either[GHException, A], statusCode: Int, headers: Map[String, String])

That way we don't lose status code and headers on the unhappy path.

Also, error handling should be front and center on the getting started.

@dcsobral
Copy link
Contributor

dcsobral commented Mar 9, 2020

As a new user, going through the documentation I kept seeing examples that simply matched on left and right, and printed that. It was never clear to me whether "right" was a complete success, or simply a lack of exceptional conditions such as connect failures. Or, put another way, were 400 and 404 rights or lefts? Did I need to handle a 30x?

I started out thinking if I got a right, then I was getting an authorized response to the API call I made, but looking at how status code was present only on right, I started to question that assumption.

So, from my perspective, that's what I wish was made clear.

@dcsobral
Copy link
Contributor

By the way, somewhat related but not totally, right now GHResponse is an alias for Either. However, it doesn't have the type classes from cats that Either does. For example, I can't replace the Either in the following code with GHResponse:

  private def autoPaginate[F[_]: Sync, T]
      (call: Pagination => F[Either[GHException, GHResult[List[T]]]]): EitherT[F, GHException, List[T]] = {
    for {
      firstPage <- EitherT(call(Pagination(1, 100)))
      pages = getPages(firstPage).map(Pagination(_, 100))
      restPages <- EitherT(pages.traverse(call(_)).map(_.sequence))
    } yield firstPage.result ++ restPages.flatMap(_.result)
  }

Whatever path you take, it would be useful to have those available.

@BenFradet
Copy link
Contributor Author

@BenFradet
Copy link
Contributor Author

this is done closing

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

No branches or pull requests

2 participants