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

Improved reporting and error handling #66

Merged
13 commits merged into from
Nov 4, 2016
Merged

Improved reporting and error handling #66

13 commits merged into from
Nov 4, 2016

Conversation

ghost
Copy link

@ghost ghost commented Jul 26, 2016

@raulraja please take a look and let me know your thoughts.

  • Ensure that fetchMany is never called with duplicated identities

I've modified the interpreter for storing information about execution rounds: the request, result and timing of each request is stored in the environment. Cached requests do no longer count as rounds, so it maps better to the mental model of a fetch execution.

I also modified the error type to be a FetchError (which is now open to extension) and discerned between a single identity that's missing NotFound and potentially multiple missing identities by batching or parallel fetching MissingIdentities. I also wrapped exceptions thrown in data source client code in a FetchException type.

This will require a few changes in the Fetch exercises, I can do that after merging this PR.

@codecov-io
Copy link

codecov-io commented Jul 26, 2016

Current coverage is 69.74% (diff: 71.30%)

No coverage report found for master at 69117ea.

Powered by Codecov. Last update 69117ea...bc7e679

def missingIdentities(cache: DataSourceCache): List[I]
def dataSource: DataSource[I, A]
def identities: NonEmptyList[I]
}

trait FetchError extends Throwable with Product with Serializable
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 not sure FetchError is a good name, I'd call it FetchException as Error indicates potentially a real JVM Error when I read it and I would not dare to catch that.

@raulraja
Copy link
Contributor

Sorry for the late review, so far what I've seen LGTM, Can't wait to get the Fetch lib published in Scala Exercises :)

peterneyens and others added 4 commits November 2, 2016 11:02
Replace `Cont[FM, ?]` monad trick with interpreter using `EitherT` and
`Writer`.
Finish cats 0.7.2 update and change `deps` method
- Refactor interpreter: split out handling of `FetchOne`, `FetchMany`
  and `Concurrent`, use `XorT` in `processMany` to reduce nested folds,
  use `Validated` in `processMany` and `processConcurrent` to get
  missing ids/identities or some result.
- (Prematurely ?) optimize `Fetch.join`
- Replaced custom implementations of `map2`, `sequence` and `traverse`
  by using `Applicative[Fetch]`
- ...

I moved some methods around in the `Fetch` object and with the
extraction of the three methods in the interpreter, so the diff looks
much bigger than it actually is.
Refactor and use some more cats features
@ghost ghost merged commit b590c80 into master Nov 4, 2016
@ghost ghost deleted the reporting branch November 4, 2016 12:35
This pull request was closed.
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.

3 participants