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

More flexible upickle integration #1969

Merged
merged 3 commits into from
Oct 12, 2023
Merged

More flexible upickle integration #1969

merged 3 commits into from
Oct 12, 2023

Conversation

adamw
Copy link
Member

@adamw adamw commented Oct 11, 2023

Closes #1821

@adamw adamw requested a review from kciesielski October 11, 2023 09:38
@adamw
Copy link
Member Author

adamw commented Oct 11, 2023

@szymon-rd what do you think? :)

import sttp.client4._
import sttp.client4.internal.Utf8
import sttp.model.MediaType
import sttp.client4.json._

trait SttpUpickleApi {
val upickleApi: upickle.Api
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Can't we just have a default implementation = upickle.default here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could but that wouldn't be very helpful. The type of the upickleApi needs to be overriden to the singleton type of the value (I'll extend the docs). Otherwise, things don't work as expected, as the compiler can't lookup the correct Reader/Writer values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, the value type has to be overridden by a concrete type. This dependent type management on the library user side is quite confusing.

@szymon-rd
Copy link
Contributor

It looks good and works very similarly to the upickle itself. I wonder if it will always type check, given using the path-dependent types in this trait.

@adamw
Copy link
Member Author

adamw commented Oct 11, 2023

@szymon-rd that's a good question, but I'm not sure how to verify if that's the case :) In a way, you're always using path-dependent types with uPickle (by importing upickle.default), so I would say it should work fine.

@adamw
Copy link
Member Author

adamw commented Oct 12, 2023

@szymon-rd I'd give this a go, however note that it will require changing imports (in the tutorials etc.) the import from sttp.client4.json.upickle._ to sttp.client4.json.upickle.default._

But that's probably version in the toolkit? (that is, a specific version of the toolkit has its own docs & fixed sttp/upickle versions)

@szymon-rd
Copy link
Contributor

@adamw We have one version of tutorials right now, we will add different versions when adoption grows. However, now, it is sufficient to just keep the most recent one.

@adamw adamw merged commit 2df769e into master Oct 12, 2023
@mergify mergify bot deleted the upickle-custom branch October 12, 2023 13:58
@adamw
Copy link
Member Author

adamw commented Oct 12, 2023

ok, releasing as 4.0.0-M6 :) we can always revert in case of problems

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

Successfully merging this pull request may close these issues.

Investigate why a separate upickle integration is needed when using non-standard serialization
3 participants