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

Use String instead of ExactError #25

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Use String instead of ExactError #25

wants to merge 2 commits into from

Conversation

nomisRev
Copy link
Member

@nomisRev nomisRev commented Jun 6, 2023

No description provided.

Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good simplification 👍The ExactError was pretty useless

@CLOVIS-AI
Copy link

I'm not a fan of using Strings as an error representation. It means translation must happen in the domain layer, which I don't think is right. I think it would be better if it was possible for users to declare their own error types.

@ILIYANGERMANOV
Copy link
Collaborator

I'm not a fan of using Strings as an error representation. It means translation must happen in the domain layer, which I don't think is right. I think it would be better if it was possible for users to declare their own error types.

Hi @CLOVIS-AI , it's possible we have ExactEither (if I remember the name correctly) which lets the user define their generic error type E.

In the case where the user is too lazy to define a custom error - what's the benefit of wrapping a String into ExactError? I don't see the value of that - it's just some extra boilerplate.

@nomisRev
Copy link
Member Author

nomisRev commented Jun 6, 2023

what's the benefit of wrapping a String into ExactError? I don't see the value of that - it's just some extra boilerplate.

I think there was probably some confusion here indeed. ExactError<Error, Input, Output> is the base interface and Exact<Input, Output> is a definition on top. Actually we've defined Exact<Input, Output> as:

fun interface Exact<Input, Output> : ExactEither<String, Input, Output>

but it should probably just be:

typealias Exact<Input, Output> : ExactEither<String, Input, Output>

I am not a huge fan of the name ExactEither but there is no better way to define it 😅 but think of Exact<Input, Output> as the typed alternative to require where String is the message of the exception.

@ustitc
Copy link
Collaborator

ustitc commented Jun 7, 2023

I had similar idea, but the only thing that stopped me is a returned Either<String, X> type. Would it be clear for clients of the library, that left part, and string in it, is an error? It can be especially misleading for cases when we deal with Exact<String, X>.

But maybe it is a matter of learning and getting used to the library.

Btw, ExactError may become useful, if we would have an initial value inside it

class ExactError<T>(val error: String, val value: T)

val value = "1235123"

Name.from(value).mapLeft {
  NotName(it.value) // NotName("1235123")
}

@ustitc
Copy link
Collaborator

ustitc commented Jun 19, 2023

We can mimic require api from std. We already have ensure(condition: Boolean) and now we need to add ensure(condition: Boolean, lazyMessage: () -> String).

In that case clients won't need to construct ExactError themselves, and it will be also clear from the function variables what is needed. So the clients will have 3 options:

  1. ensure(condition: Boolean)
  2. ensure(condition: Boolean, lazyMessage: () -> String)
  3. ensure(raw: T, raise: () -> ExactError)

Using 1 and 2 will allow clients to write less boilerplate code and we will have a type that states clear that we deal with an error, not a generic string (we are a library about concrete types after all 😄 ). I think as a next step we can hide the 3rd option, because it shouldn't be used that often.

I would also suggest:

  1. Rename ExactError to ErrorMessage
  2. Make it value class

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.

4 participants