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

Add support for jsoniter Json #1248

Merged
merged 10 commits into from
Jan 24, 2022
Merged

Add support for jsoniter Json #1248

merged 10 commits into from
Jan 24, 2022

Conversation

borissmidt
Copy link
Contributor

@borissmidt borissmidt commented Jan 9, 2022

Before submitting pull request:

  • [x ] Check if the project compiles by running sbt compile
  • Verify docs compilation by running sbt compileDocs
  • Check if tests pass by running sbt test
  • Format code by running sbt scalafmt

@borissmidt
Copy link
Contributor Author

Offtopic; I also updated the openApi generator to use sttp3.
OpenAPITools/openapi-generator#11260

@Pask423
Copy link
Contributor

Pask423 commented Jan 12, 2022

Could you merged newest master, it should solve problem with js tests.
It would be also great if you could write docs for jsoniter similar to one written for zio-json.
Please check if all files are correctly formatted with fmt.

@borissmidt
Copy link
Contributor Author

I see one big problem with jsoniter and sttp and that is that sttp uses either and option a lot.
The it is possible in jsonIter to handle the option with the implicit def.
But the Either isn't really that safe so the user has to manually define an implicit val for every variant of either he wants to use.

So i think jsoniter needs to be improved for our usecase.

Refactored code
added option def to jsonIterApi
@Pask423
Copy link
Contributor

Pask423 commented Jan 12, 2022

That can be quite problematic from user perspective. I am not that familiar with jsoniter to propose any workaround for this.
BTW I still see some merge conflicts could you resolve them ?

@borissmidt
Copy link
Contributor Author

I could only make a workaround for 'option[t]'.
So i think changes have to be made to jsoniter before we can support it in this library.

For Either i couldn't really make a good workaround.

So we might better close the pr and maybe we can best keep the github issue open and comment these issues on there for future reference?

@adamw
Copy link
Member

adamw commented Jan 14, 2022

Hm I don't think we need any special Option/Either support. The options/eithers that appear in signatures are used to represent cases where either JSON parsing failed, or we received a non-200 response. So we don't typically need json codecs for these types, only for whatever is the "root" type.

Removed tests that are expect ambiguas either objects.
@borissmidt
Copy link
Contributor Author

I've removed the ambiguous Either tests and now i expect the use of asJsonEither so it is clear that you either have an error of type Left or a body of type Right.

@Pask423
Copy link
Contributor

Pask423 commented Jan 19, 2022

Great just resolve conflicts and I think we can merge it

@Pask423
Copy link
Contributor

Pask423 commented Jan 20, 2022

There is an error with downloading dependency
image

@borissmidt
Copy link
Contributor Author

scala 3 macro support still needs to be added. I'll remove scala3 from the possible builds.

@adamw adamw merged commit 54a39f9 into softwaremill:master Jan 24, 2022
@Pask423 Pask423 mentioned this pull request Jan 24, 2022
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.

3 participants