-
Notifications
You must be signed in to change notification settings - Fork 418
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
Schema based ops for en- and decoding bodies #2569
Conversation
6b2f76e
to
ff1039f
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2569 +/- ##
=======================================
Coverage 64.23% 64.24%
=======================================
Files 140 140
Lines 8358 8357 -1
Branches 1604 1586 -18
=======================================
Hits 5369 5369
+ Misses 2989 2988 -1 ☔ View full report in Codecov by Sentry. |
* val decodedPerson = body.as[Person] | ||
* }}} | ||
*/ | ||
def as[A](implicit codec: BinaryCodec[A], trace: Trace): Task[A] = |
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.
Should this be to
or into
? as
has a meaning in some ZIO contexts as map(_ => a)
.
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 chose the name because it is called queryAs
if you want to get a typed query param
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.
Actually I am fine with both. I can change it next time a PC is in reach
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.
Maybe that should be queryTo
or queryInto
for consistency?
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.
Should it then also be body.toString
and not body.asString
? But this has an obvious conflict.
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.
Agreed, please update any other cases like this that you find.
* val body = Body.from(person) | ||
* }}} | ||
*/ | ||
def from[A](a: A)(implicit codec: BinaryCodec[A], trace: Trace): Body = |
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 fact that this is called from
almost does imply to
or into
.
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.
Good to merge with a name change, including one to queryAs
(queryTo / queryInto). Thanks for your work on this!
ff1039f
to
ea9cc05
Compare
@jdegoes please take a look again. I changed too much to feel go to merge without an additional review |
Currently, we leave transforming data from and to bodies more or less to the user.
We only understand basic types like
String
orChunk[Byte]
.But since we have the endpoint API and a strong relationship with zio-schema, I think it makes sense, to have schema based operators, that can create and decode bodies.
This implementation is agnostic of the encoding (not json specific) and just requires an implicit
BinaryCodec
.