(dsl): Support GetById request #5
Conversation
drmarjanovic
force-pushed
the
dsl-get-by-id-request
branch
from
494196f to
60f18cd
Compare
modules/library/src/main/scala/zio/elasticsearch/ElasticRequest.scala
Outdated
Show resolved
Hide resolved
drmarjanovic
force-pushed
the
dsl-get-by-id-request
branch
from
40f0273 to
849b849
Compare
drmarjanovic
force-pushed
the
dsl-get-by-id-request
branch
from
849b849 to
91a344c
Compare
| routing: Option[Routing] = None | ||
| ) extends ElasticRequest[Option[Document]] | ||
|
|
||
| sealed abstract class DocumentGettingError |
There was a problem hiding this comment.
I would probably extract errors to a separate file. Also, I would name it more generally. Not just DocumentGettingError. Either ElasticError or something like that. @arnoldlacko, any thoughts?
There was a problem hiding this comment.
I would avoid having one enum for all errors, because it reduces the benefits of typed errors. For example put, which will not be able to fail with DocumentNotFound I believe should have an error type that does not have this error as a possibility. If we only have ElasticError, a user calling a put will have to deal with DocumentNotFound or make assumptions that are not enforced by the type system.
I think we could find better names for the error groups for sure, but I wouldn't merge them into one.
There was a problem hiding this comment.
Perhaps for now we can expect to have 3 abstract error types: GetError, PutError, QueryError?
There was a problem hiding this comment.
Regarding the extraction into a separate file, I think that makes sense.
|
|
||
| case object DocumentNotFound extends DocumentGettingError | ||
|
|
||
| case class JsonDecoderError(errorMsg: String) extends DocumentGettingError |
There was a problem hiding this comment.
- I prefer
final case class. - Can you use
messageorreasoninstead?
|
|
||
| object ElasticError { | ||
|
|
||
| sealed abstract class DocumentGettingError |
There was a problem hiding this comment.
What do you think about having only sealed trait ElasticError at the moment?
final case object DocumentNotFound extends ElasticError
final case class DecoderError(reason: String) extends ElasticError
We should consider this because we don't have a lot of errors, and maybe it's too early to introduce several layers of error types.
|
|
||
| final case object DocumentNotFound extends DocumentGettingError | ||
|
|
||
| final case class JsonDecoderError(message: String) extends DocumentGettingError |
There was a problem hiding this comment.
I would omit Json prefix here, and possibly change message to reason(s).
No description provided.