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

Streamline instances #2210

Merged
merged 10 commits into from Nov 2, 2018
Merged

Streamline instances #2210

merged 10 commits into from Nov 2, 2018

Conversation

@aeons
Copy link
Member

@aeons aeons commented Oct 27, 2018

Continuing on from the discussions in #2203, this PR is an exploration that does the following:

  • Deprecates Http4s, Http4sInstances, and Http4sFunctions
  • Moves instances for cats typeclasses into the corresponding classes' companion objects
  • Makes Http4sDsl extends UriFunctions, EntityEncoderInstances and EntityDecoderInstances instead of Http4s*
  • Updates tests and examples

I put the single function in QValueFunctions into the QValue object. I wanted to do the same for UriFunctions, but Uri.uri is used in a lot of places and examples unqualified, so I kept it and added it to the DSL and some test traits (but moved the two other functions in the trait).
If the DSL should still bring in the uri function, that could be solved by adding it directly in the DSL trait (since it's a macro invocation, it can't just be called), so the UriFunctions trait could be removed.

EntityEncoderInstances and EntityDecoderInstances are mixed into the DSL, but when not using that, getting both of them in scope requires imports. I'm fine with that, and I think I might even prefer imports.

Since some of the traits mixed into the deprecated traits have been removed, it might cause breaking changes, but there will be a deprecation notice at least.

Closes #2203

Copy link
Member

@rossabaker rossabaker left a comment

I think the *Functions traits are anachronistic. I don't see that approach much anymore. I also don't see any good reason it should be on the server DSL trait. It's probably of more use in the client DSL, but it's hard to say where we should draw the line for importing constructors once we start.

I didn't even realize we were mixing in EntityEncoderInstances and EntityDecoderInstances. As long as they're in the companion objects, the implicit ones are already in scope. I think it's fine to need to import the explicit ones.

@ChristopherDavenport ChristopherDavenport added this to the 0.20.0 milestone Oct 29, 2018
@aeons aeons force-pushed the aeons:instances branch from 993b840 to 7f63b51 Nov 1, 2018
@aeons
Copy link
Member Author

@aeons aeons commented Nov 1, 2018

I rebased and fixed the Order instance for HttpVersion, which made the tests fail.

Http4sDsl and Http4sSpec does not mix in any Entity*Instances nor UriFunctions any more.

We could go one step further and remove the separate Entity*Instances traits and just have the instances in the companion object.

aeons added 2 commits Nov 1, 2018
@aeons aeons mentioned this pull request Nov 1, 2018
Copy link
Member

@rossabaker rossabaker left a comment

I don't think exposing EntityInstances as separate public traits makes sense. Where it helps prioritization, a stack of private traits makes sense. Otherwise, I would just put them on the companion.

That could be bolted onto this PR or addressed in a separate one. No preference.

@aeons
Copy link
Member Author

@aeons aeons commented Nov 1, 2018

I actually thought I folded the instances into the companion objects here, I'll push that in a bit.

@aeons aeons force-pushed the aeons:instances branch from 8880a33 to 2eb4575 Nov 1, 2018
@aeons aeons merged commit 8bd7cd3 into http4s:master Nov 2, 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
@aeons aeons deleted the aeons:instances branch Nov 2, 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.

None yet

3 participants