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 MediaType literal parsing #2508

Merged
merged 2 commits into from Apr 16, 2019

Conversation

Projects
None yet
3 participants
@rossabaker
Copy link
Member

left a comment

I can't remember why we're doing functions instead of StringContexts here. Also can't remember why these had to be whitebox instead of blackbox. Things to think about for the future, but this looks like progress.

@bpholt bpholt force-pushed the Dwolla:mediatype branch from c028160 to b176b4c Apr 13, 2019

@bpholt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

Good idea to use StringContext. I added them for Uri, MediaType, and QValue. (The QValue one uses qValue"" as the syntax instead of just q"" since the latter is well-known for quasiquotes.) Let me know what you think.

One of the blaze-client tests is failing locally for me, but I'm not sure it's related to my changes. We'll see what happens in CI.

add StringContext-based interpolators for Uri, MediaType, and QValue
deprecates the old whitebox-macro-based functions

@bpholt bpholt force-pushed the Dwolla:mediatype branch from b176b4c to c008bc4 Apr 13, 2019

@rossabaker

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

Which one failed locally? Local failures of those are my holy grail.

@@ -375,6 +375,7 @@ object Uri {
* Literal syntax for URIs. Invalid or non-literal arguments are rejected
* at compile time.
*/
@deprecated("""use uri"" string interpolation instead""", "0.20")

This comment has been minimized.

Copy link
@rossabaker

rossabaker Apr 13, 2019

Member

As discussed, this one is going to cause the most migration pain, but it's like what we'll be doing in the long run.

@@ -100,4 +100,11 @@ package object http4s { // scalastyle:ignore
case Left(l) => f(l)
}
}

final implicit class Http4sLiteralSyntax(val sc: StringContext) extends AnyVal {

This comment has been minimized.

Copy link
@rossabaker

rossabaker Apr 13, 2019

Member

Is this "syntax"? Should it be imported through org.http4s.implicits._ rather than be part of the package object? My gut feeling is yes, but ip4s does it at the top level.

@mpilquist, is implicits a design you considered and rejected?

@bpholt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

Which one failed locally? Local failures of those are my holy grail.

[error]   x behave and not deadlock on failures with parSequence (30 seconds, 207 ms)
[error]    None is not Some (BlazeClientSpec.scala:189)
[error] org.http4s.client.blaze.BlazeClientSpec.$anonfun$new$31(BlazeClientSpec.scala:189)
@rossabaker

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

If that's consistent for you, I'd love to hear more about your machine.

@bpholt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

That test failed probably 3-4 times in a row for me the other day, but it's passing now. 🤷‍♂️

@rossabaker

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Any second reviewer before I merge?

@rossabaker rossabaker merged commit 0e50822 into http4s:master Apr 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bpholt bpholt deleted the Dwolla:mediatype branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.