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

Replace Result type with cats.Validated #49

Open
muuki88 opened this issue Sep 11, 2018 · 1 comment
Open

Replace Result type with cats.Validated #49

muuki88 opened this issue Sep 11, 2018 · 1 comment

Comments

@muuki88
Copy link
Owner

muuki88 commented Sep 11, 2018

No description provided.

@felixbr
Copy link
Collaborator

felixbr commented Jul 12, 2019

I looked through the code to see if this issue is still needed.

So far it looks like Result is type Result[T] = Either[Failure, T] and Failure is just a wrapper around an error message.

Imo that's not perfect, but also not too bad atm. Replacing Either with ValidatedNel only makes sense for independent steps, but a lot of things are sequential in nature, so Either is fine as a default. If there are things where accumulating errors is possible, using ValidatedNel and converting the end result to Either is easy enough now (see Preprocessors.apply).

So there seem to be two questions left:

  1. Is the type-alias worth it or would it be clearer to just use Either and ValidatedNel directly.
  2. Would it make sense to have error-ADTs describing the different failure-cases as types (which would maybe also require (1) )

My personal thought is that an error-ADT would mostly be useful for testing, but the end-user of the plugin just wants decent error-messages, so the current solution could be sufficient. If we only transport error-messages we may as well keep the type-alias as it is now.

Just my 2 cents 🙂

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