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

Weaken Http/HttpRoutes Constraint #2906

Merged
merged 2 commits into from Oct 29, 2019

Conversation

@ChristopherDavenport
Copy link
Member

ChristopherDavenport commented Oct 10, 2019

Sync is no longer required, we only need Delay for the ability to suspend the route analysis.

@aeons
aeons approved these changes Oct 29, 2019
@aeons aeons merged commit 81694fe into http4s:master Oct 29, 2019
2 checks passed
2 checks passed
Summary no rules match, no planned actions
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rossabaker

This comment has been minimized.

Copy link
Member

rossabaker commented Oct 30, 2019

We had a previous discussion of this at #2325 and decided not to do it. I'm fine with this, but curious: did something change in the library, or just in our attitude?

@aeons

This comment has been minimized.

Copy link
Member

aeons commented Oct 30, 2019

I completely forgot that discussion, and thought that weaker constraints => better.
I guess I still agree with the points made there, so revert this one?

@ChristopherDavenport

This comment has been minimized.

Copy link
Member Author

ChristopherDavenport commented Oct 30, 2019

The combination of this and the circe typeclass model means that you no longer need a Sync at the edge of the application. If you are decoding Json or do similar with something else you can have a weak constraint on your edge.

Yes you need it for the server, but your routes don't need to have such strong constraints.

#2907

@hamnis

This comment has been minimized.

Copy link
Contributor

hamnis commented Oct 31, 2019

@ChristopherDavenport Any particular reason AuthedRoutes did not get the same treatment?
I can fix this as part of #2612 if this is interesting.

@ChristopherDavenport

This comment has been minimized.

Copy link
Member Author

ChristopherDavenport commented Nov 1, 2019

Nope, please feel free to integrate there as well.

hamnis added a commit to hamnis/http4s that referenced this pull request Nov 1, 2019
@rossabaker

This comment has been minimized.

Copy link
Member

rossabaker commented Nov 2, 2019

Okay, I'll buy that, and 👍 to keeping AuthedRoutes consistent.

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