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

Adding a simple Stream as JSON array circe encoder #3056

Merged
merged 3 commits into from Jan 12, 2020
Merged

Adding a simple Stream as JSON array circe encoder #3056

merged 3 commits into from Jan 12, 2020

Conversation

BalmungSan
Copy link
Contributor

@BalmungSan BalmungSan commented Jan 10, 2020

This adds some functions to create a simple encoder for Streams, which encodes it as a single JSON array. The specs show how they work.

I also added the option to specify which charset to use when encoding a JSON to all the previous functions, since the method has a default argument, I guess it is binary compatible?
I am actually pretty bad at that. If the change is dangerous I may remove it.

I am open to any feedback you may have about these changes.
(btw, I tried to follow the style conventions that were already there).

@BalmungSan
Copy link
Contributor Author

@BalmungSan BalmungSan commented Jan 10, 2020

As a side note, I have read somewhere that mixing Context Bounds with normal implicit parameters is not recommended. However, my changes do that, but just because the rest of the file also does it, and I tried to be consistent.
But, IMHO, it would be good to rewrite all that to just use implicit parameters.

@BalmungSan
Copy link
Contributor Author

@BalmungSan BalmungSan commented Jan 10, 2020

It seems a test failed, but as far as I can tell, the test that failed has nothing to do with my change.
Also, I am pretty sure all test passed in my first commit (I amened some scalafmt issues), so it seems the problem is non-deterministic.
Did you already know about this problem?

@bfdes
Copy link
Contributor

@bfdes bfdes commented Jan 11, 2020

I think I have a similar problem in #3054.

Copy link
Member

@rossabaker rossabaker left a comment

Thanks!

We have had a problem with a few flaky tests, particularly in blaze, on Travis CI for a long time. I think it's related to excessive use of unsafeRunSync in the tests and manifests itself on lesser powered hardware, but I've had a hard time reproducing outside Travis. I restarted the test, so not your problem.

I agree that mixing context bounds and implicits is bad (I wish there were a scala flag for this), and would support fixing those as well.

@BalmungSan
Copy link
Contributor Author

@BalmungSan BalmungSan commented Jan 11, 2020

I agree that mixing context bounds and implicits is bad (I wish there were a scala flag for this), and would support fixing those as well.

Would you prefer that on this PR, or in a following PR with only aesthetic changes?
Also, I am not sure if that mix happens elsewhere? Maybe it would be worth creating a new issue to track all files with such mixture and all PRs fixing those.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 11, 2020

I'd bet it does happen elsewhere, but I don't know how to find them. I'm sure there's some scalameta wizardry that would help.

A separate PR sounds good for fixing this file.

@BalmungSan
Copy link
Contributor Author

@BalmungSan BalmungSan commented Jan 11, 2020

@rossabaker I just pushed the change removing the use of Charset and adding the constant chunks.

Let me know if you have any other suggestions.

Copy link
Member

@rossabaker rossabaker left a comment

Looks good. Thanks!

hamnis
hamnis approved these changes Jan 12, 2020
@BalmungSan
Copy link
Contributor Author

@BalmungSan BalmungSan commented Jan 12, 2020

@rossabaker I moved the constant byte chunks into the companion object, so they aren't instantiated for each instance. And I also cached the creation of the open and close brace streams.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 12, 2020

Looks great. The test failure is unrelated, which I'm trying to mitigate in #3058, because I don't think it describes a real issue.

@rossabaker rossabaker merged commit 3d1ef03 into http4s:master Jan 12, 2020
1 of 2 checks passed
@BalmungSan BalmungSan deleted the circe-json-array-stream-encoder branch Jan 12, 2020
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.

None yet

4 participants