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 to JSON encoding and decoding #140

Merged
merged 6 commits into from
Jan 25, 2024
Merged

Conversation

ubaldop
Copy link
Contributor

@ubaldop ubaldop commented Jan 19, 2024

Hi, this is an attempt to add support to JSON data type as well.

Thanks @satabin for the support 😸

@ybasket
Copy link
Contributor

ybasket commented Jan 19, 2024

Very cool, thank you @ubaldop. I won't have time for a proper review now, but will get back to it in the coming days!

In the meantime, I approved the first CI run and it failed as the workflows are not up-to-date. Run sbt githubWorkflowGenerate locally and push the changes – I'll see to press the button again so we have a proper CI run.

Copy link
Contributor

@ybasket ybasket left a comment

Choose a reason for hiding this comment

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

Looks good, left only one comment for a small config update.

What would be great in addition is a standalone example in the docs, like we have for the other formats. Helps people a lot to get started using your work. Would you be up for adding that as well?

build.sbt Outdated
Comment on lines 105 to 106
startYear := Some(2023),
tlVersionIntroduced := Map("2.12" -> "0.2", "2.13" -> "0.2", "3" -> "0.2"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
startYear := Some(2023),
tlVersionIntroduced := Map("2.12" -> "0.2", "2.13" -> "0.2", "3" -> "0.2"),
startYear := Some(2024),
tlVersionIntroduced := Map("2.12" -> "0.3", "2.13" -> "0.3", "3" -> "0.3"),

Let's update the year and set the correct version (will release this PR as 0.3 once merged). Run sbt headersGenerate as it might change the file headers.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't think this is correct. The tlVersionIntroduced should always be a full version e.g. 0.3.1. This is only needed when introducing a new module in an existing binary-compatibility series.

will release this PR as 0.3 once merged

In that case, you should bump the tlBaseVersion to 0.3 in this PR. In general, the tlBaseVersion should be bumped in the PR that makes the changes that cause you to decide to bump the version.

Then tlVersionIntroduced won't be necessary since it's a new binary-compatibility series.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thank you for chiming in! I hope I understood them correctly in 96bccaa.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly right!

@ubaldop
Copy link
Contributor Author

ubaldop commented Jan 24, 2024

Looks good, left only one comment for a small config update.

What would be great in addition is a standalone example in the docs, like we have for the other formats. Helps people a lot to get started using your work. Would you be up for adding that as well?

Sure, I am on it!

@ybasket
Copy link
Contributor

ybasket commented Jan 25, 2024

Sure, I am on it!

I like the pretty printing example! Just pushed a commit that fixes the site build (dependency was missing). Will merge once CI is happy and then release together with the open dependency updates soon.

@ubaldop
Copy link
Contributor Author

ubaldop commented Jan 25, 2024

I like the pretty printing example!

Thanks! I thought it was the easiest way to show both decoding and encoding in action with few lines of code 😸

@ybasket ybasket merged commit 005abdf into http4s:main Jan 25, 2024
14 checks passed
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