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

Removing charset for argonaut encoders #1914

Conversation

@calvinbrown085
Copy link
Contributor

@calvinbrown085 calvinbrown085 commented Jun 6, 2018

fixes #1878

We wanted to keep consistency with our Content Type headers across JSON implementations.

@calvinbrown085 calvinbrown085 changed the title Removing content type for argonaut encoders Removing charset for argonaut encoders Jun 6, 2018
@calvinbrown085
Copy link
Contributor Author

@calvinbrown085 calvinbrown085 commented Jun 6, 2018

@ChristopherDavenport @rossabaker Would you guys mind taking a look here to make sure I did the correct thing?

jsonEncoder.headers.get(`Content-Type`) must_== None
"have json content type" in {
jsonEncoder.headers.get(`Content-Type`) must_== Some(
`Content-Type`(MediaType.application.json))

This comment has been minimized.

@rossabaker

rossabaker Jun 6, 2018
Member

Wow. I'm trying to remember the justification for the old test. Why hasn't it always been this way?

@@ -34,21 +34,21 @@ class ArgonautSpec extends JawnDecodeSupportSpec[Json] with Argonauts {
val custom = ArgonautInstances.withPrettyParams(PrettyParams.spaces2)
import custom._
writeToString(json) must_== ("""{
| "test" : "ArgonautSupport"
|}""".stripMargin)
| "test" : "ArgonautSupport"

This comment has been minimized.

@aeons

aeons Jun 6, 2018
Member

Hmm, why is scalafmt happy with these changes?

This comment has been minimized.

@calvinbrown085

calvinbrown085 Jun 6, 2018
Author Contributor

@aeons Not sure, I did a ;test;scalafmt and it passed.

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Jun 7, 2018
Member

I would prefer perhaps these be defined above and then checked against so the formatting could be cleaner. As that inset is not fun to read at the end of a test.

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

Looks good to me, the formatting improvements I would prefer but those are not blocking.

@@ -34,21 +34,21 @@ class ArgonautSpec extends JawnDecodeSupportSpec[Json] with Argonauts {
val custom = ArgonautInstances.withPrettyParams(PrettyParams.spaces2)
import custom._
writeToString(json) must_== ("""{
| "test" : "ArgonautSupport"
|}""".stripMargin)
| "test" : "ArgonautSupport"

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Jun 7, 2018
Member

I would prefer perhaps these be defined above and then checked against so the formatting could be cleaner. As that inset is not fun to read at the end of a test.

@calvinbrown085
Copy link
Contributor Author

@calvinbrown085 calvinbrown085 commented Jun 14, 2018

@ChristopherDavenport Not sure what you mean there

@ChristopherDavenport
Copy link
Member

@ChristopherDavenport ChristopherDavenport commented Jun 14, 2018

Unimportant. No worries.

@ChristopherDavenport ChristopherDavenport merged commit 094ef9f into http4s:master Jun 14, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@calvinbrown085 calvinbrown085 deleted the calvinbrown085:calvinb-remove-json-content-type-encoder-argonaut branch Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants