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.
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
Custom body decoding #555
Custom body decoding #555
Changes from 4 commits
733178f
f0b44c6
32b2bcf
c83fdcc
64e62d4
f392539
8541522
b3bfa99
039d0b6
f171e71
ca6fb44
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.
Looks like a typo with 2
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.
I believe is actually valid KDoc where you can specify the display text for a link π€
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, it is deliberate. It allows to link to
body
property and start the sentence with a capital letter.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.
nit: This is more an API design question.
What we're modelling here is an operation that could have 3 outcomes: success (
String
), failure (IOException
), skipped (null
).Have you considered different approaches (e.g.
sealed classes
or the KotlinResult
type)? I'm not saying that the approach here is wrong, but I was wondering if we have some benefit in modelling it differently.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.
Agree with this nitpick that it might be a good idea to model the result to leave less ambiguity.
For example, we could have something like
Failure
instead of null in case of unsupported content.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, I considered it and I don't think it is the right choice. I don't see how it helps us in the processing pipeline. We are interested only in the first successfully decoded body.
IOException
does not model a type of respons. Methods are annotated with it because tools like adapters generally can throw this type of exception so it removes form users a burden of needing to deal with it. And in Kotlin it is easy to forget since exceptions are not checked. Also, it allows us to log exceptions for the user so they have feedback information when something goes wrong. I would be either for keeping this part as is or to remove@Throws
and move the responsibility to the users.Returning
null
instead of some type is a nod towards libraries like Retrofit or Moshi, where factories return it when they can't handle input. I don't mind modelling it with a sealed class but I don't know if it brings anything to the table. If you feel that it would be helpful I'll change 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.
Thanks for your insights π I think we're fine with the current modelling for the reasons you mentioned + is the one that has the smaller API surface
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.
This thread started with
nit
annotation, so let's leave it as it is :)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.
Same typo with 2
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.
Same, not a typo. :)