This repository has been archived by the owner on Feb 24, 2021. It is now read-only.
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.
Suspend Validated.fx implementation #131
Suspend Validated.fx implementation #131
Changes from 5 commits
ea21499
365288c
f2ac3a4
4b79a7a
2b4fce8
5c3cfd9
2402bf9
41e6dbb
5ce5acb
ddad46a
e7c6029
ad05142
ef46255
18d4c35
923e6cd
8dfcf98
d046524
d88425c
0d24068
794af4d
840576c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Instead of
Throw
, we could return an Invalid with it.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.
But that's the impossible case, does it really matter? Actually I was looking for if there was an operator that does this already and doesn't leave the burden of writing this to the dev but couldn't see any
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.
Isn't it exhaustive then?
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.
It cannot be because we're checking 2 elements
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.
we need pattern matching weep. I'd still use Invalid for the sake of not promoting exceptions. Not a blocker tho.
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.
Yes, you can use
Applicative
for this.You can always make these exhaustive by nesting
when
, which after a lot of back and forth have gone back to...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.
Right, I just realized that we're in core-data and we don't have access to the extensions :/
What I did is the following:
nelApplicative
function in Validated extensionsThere 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.
It's just meh in general. It could be possible to rewrite it as two nested folds and some manual formatting.
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.
I'm not a fan of showing
parallelValidate
at all.. It promotes/shows code that no-one should ever write using Arrow.We have a bunch of
helpers
in thedocs
module, any way we can provide instances there to make the desired snippets compile?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.
The only reason I like it is for the explanation vs a Validated Nel, which is also explained in there. I changed it to try to make it clear that this is just for the shake of the example
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.
Neat
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.
Great! Good use of both APIs