docs.scala-lang icon indicating copy to clipboard operation
docs.scala-lang copied to clipboard

Change tutorial to use ujson integr.

Open szymon-rd opened this issue 2 years ago • 9 comments

szymon-rd avatar Jul 26 '23 10:07 szymon-rd

originally I explored this a lot and it seems like maybe its too much magic (particularly when parsing the response body) - because now it returns either rather than throw exception,

maybe its more acceptable for writing the request body?

bishabosha avatar Jul 26 '23 10:07 bishabosha

maybe its more acceptable for writing the request body?

It is indeed more acceptable for writing the request body. But then I don't like that it is not symmetrical: we use usjson.read to read the response body but we don't use ujson.write to write the request body.

IMO the current documentation is "simple" enough and clear. It's symmetrical between request and response, and we can compare it with writing and reading to files too. There is no oslib-upickle integration so why would we need an sttp-upickle integration. It does not add anything really useful.

I don't think we should merge this PR.

adpi2 avatar Jul 26 '23 11:07 adpi2

If we merged that, it would also make sense to use the .response(asJson[...]) mechanism, like in the example for sttp-ujson integration:

basicRequest
  .post(uri"...")
  .body(requestPayload)
  .response(asJson[ResponsePayload])
  .send(backend)

szymon-rd avatar Jul 26 '23 15:07 szymon-rd

If we merged that, it would also make sense to use the .response(asJson[...]) mechanism, like in the example for sttp-ujson integration:

basicRequest
  .post(uri"...")
  .body(requestPayload)
  .response(asJson[ResponsePayload])
  .send(backend)

this also changes the body to be Either[..., ResponsePayload], which is something that could be surprising.

see https://github.com/softwaremill/sttp/issues/1771 for discussion

bishabosha avatar Jul 26 '23 15:07 bishabosha

I think that since we do have sttp<->ujson integration, we should be using that, as that's the way people should use both libraries together. I see how breaking this down might be educational, but then is the goal showing how to best use the toolkit libraries, or is the goal to explain the underlying mechanics? I think it's the former, but of course I might be wrong :)

Also I agree that then we should use the integration fully. Error handling is still something I'd like to spend some time on, and possibly refine it in sttp4. But it's a tricky thing to get right ;).

For now if we want exceptions to be thrown upon a non-2xx response, or a deserialization problem, that's the way you would do it with sttp:

basicRequest
  .post(uri"...")
  .body(requestPayload)
  .response(asJson[ResponsePayload].getRight)
  .send(backend)

Not saying it's the perfect way or that it can't be improved, but that's where the API is right now :)

adamw avatar Jul 27 '23 08:07 adamw

We have two APIs:

  1. the ujson/upickle API
val response = quickRequest
  .post(...)
  .send()

val json = ujson.read(response.body)
  1. the sttp-upickle API
val response = quickRequest
  .post(...)
  .response(asJson[ujson.Value].getRight)
  .send()

val json = response.body

In the context of the Toolkit, which is scripting and prototyping, I don't think that 2 is significantly better than 1.

More importantly, I think we should better explain that sttp and upickle combine well together rather than showing a specific API for handling JSON in HTTP requests. Otherwise the learner will get confused: "Why do I have to learn 2 different APIs, one general API to handle JSON (upickle/ujson) and one more specific for HTTP requests (sttp-upickle)?"

adpi2 avatar Jul 28 '23 11:07 adpi2

Good point, you are right that in the context of quickRequest where we always read the response into a string and not care about non-2xx responses, ujson.read(response.body) is the simpler thing to do.

Though I think you are wrong about scoping toolkit to scripting/prototyping only. Since there is an "official" set of libaries - the toolkit - people will be using that in all kinds of scenarios.

Maybe that's planned, but "advanced" tutorials which would cover - for example - error handling in http requests using response descriptions (asJson etc.) would make sense as well>

adamw avatar Jul 28 '23 12:07 adamw

Maybe that's planned, but "advanced" tutorials which would cover - for example - error handling in http requests using response descriptions (asJson etc.) would make sense as well

We do not intend to cover all the features of the Toolkit libraries in the Toolkit tutorials. The Toolkit tutorials focus mostly on straightforward solutions for scripting and prototyping. There is What else can sttp do? where we can add a section about error handling that would redirect to the sttp documentation.

adpi2 avatar Jul 28 '23 13:07 adpi2

Right, rewriting the whole docs would be quite pointless :) Adding a section on error handling to the tutorial you mention sounds great.

Though I would still think about the toolkit docs more like a starter and tutorial introducing these tools, not necessarily focused only on scripting (which is a very niche Scala use-case atm) and prototyping. I always considered it a great idea for "normal" projects as well.

adamw avatar Jul 28 '23 19:07 adamw

okay so where does this stand? should it stay open?

SethTisue avatar Oct 26 '24 19:10 SethTisue

I think Adrien made a good point and we can close this.

But @lbialy steers the Toolkit strategy now, so I am tagging him here.

szymon-rd avatar Oct 27 '24 17:10 szymon-rd