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

Confusing ResponseConverter change #71

Closed
DatL4g opened this issue Sep 28, 2022 · 1 comment
Closed

Confusing ResponseConverter change #71

DatL4g opened this issue Sep 28, 2022 · 1 comment

Comments

@DatL4g
Copy link
Contributor

DatL4g commented Sep 28, 2022

First of all make sure to correct me if I'm wrong or didn't think it through :)

Problem

The ResponseConverter change made in the latest release v1.0.0-beta14 is hella confusing and kind of "wrong".

You stated that the SuspendResponseConverter is now called RequestConverter however this is not correct.
SuspendResponseConverter is now ResponseConverter and the previous ResponseConverter is called RequestConverter.

That's the first thing but you can kind of "ignore" that when I explained the real problem.

Real problem

The RequestConverter IS NOT a request converter, it's a response converter.

What do we wanna achieve?

It's purpose is to change (convert) the response given by the HttpRequest to the desired type of object.

Example:

  • HttpRequest returns String
  • We want to convert the return type String to Flow<String>
  • Converter changes response to Flow<String>

What's the purpose of a RequestConverter?

With a request converter we want to convert a parameter or body data to another type before executing it.

Example:

Let's take the Proxmox API.
The Proxmox API handles Boolean values by Integer, that means 0 = false and 1 = true.
So if we wanna pass a Boolean parameter in the Proxmox request we actually need to pass an Integer.
That's were the request converter is handy 😄

  • We pass our paramater as Boolean
  • Converter changes Boolean to Int
  • Execute HttpRequest with the paramater changed by the request converter

TLDR

ResponseConverter -> converts HttpRequest response
RequestConverter -> converts HttpRequest parameter

The naming is simply wrong and prevents the additional improvement of adding a real RequestConverter

So I would like to request changing it back to it's original names (and switching it back in the Ktorfit builder as we need to pass a class two times when we inherit from both now).

@Foso
Copy link
Owner

Foso commented Oct 4, 2022

Hi @DatL4g thank you for your input. My intention was to distinguish between a converter that can convert a String response to a Call response, independent of it is a suspend function or non-suspend.
And a converter that can convert a suspend function to a non-suspend function.

I can see that the naming is not ideal. I will revert this change for now and and think about improving it when i released a stable 1.0

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