Improved reporting and error handling #66
Merged
Conversation
This file contains 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
Current coverage is 69.74% (diff: 71.30%)
|
| def missingIdentities(cache: DataSourceCache): List[I] | ||
| def dataSource: DataSource[I, A] | ||
| def identities: NonEmptyList[I] | ||
| } | ||
|
|
||
| trait FetchError extends Throwable with Product with Serializable |
There was a problem hiding this comment.
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.
|
Sorry for the late review, so far what I've seen LGTM, Can't wait to get the Fetch lib published in Scala Exercises :) |
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
merged commit
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@raulraja please take a look and let me know your thoughts.
fetchManyis never called with duplicated identitiesI'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 missingNotFoundand potentially multiple missing identities by batching or parallel fetchingMissingIdentities. I also wrapped exceptions thrown in data source client code in aFetchExceptiontype.This will require a few changes in the Fetch exercises, I can do that after merging this PR.