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

Typed HttpMethodOverride Middleware #2407

Merged
merged 12 commits into from Feb 14, 2019

Conversation

@ChristopherDavenport
Copy link
Member

@ChristopherDavenport ChristopherDavenport commented Feb 12, 2019

Updates with types on #2355

Otherwise the same PR

@@ -15,23 +15,23 @@ class HttpMethodOverriderSpec extends Http4sSpec {
private final val varyHeader = "Vary"
private final val customHeader = "X-Custom-Header"

private val headerOverrideStrategy = HeaderOverrideStrategy(CaseInsensitiveString(overrideHeader))
private val queryOverrideStrategy = QueryOverrideStrategy(overrideParam)
private def headerOverrideStrategy[F[_], G[_]] = HeaderOverrideStrategy[F, G](CaseInsensitiveString(overrideHeader))
Copy link
Member

@rossabaker rossabaker Feb 14, 2019

Do these F and G infer? We may have imposed a slight usability tax here. I'm for it anyway if it increases soundness, but it'd be really neat if it didn't make it harder to use. It's a little hard for me to visualize the impact with the strategies all defined at the top like this.

Copy link
Member Author

@ChristopherDavenport ChristopherDavenport Feb 14, 2019

They do not infer nicely, Not really sure what I can do to improve it and keep the type inference we have however.

Copy link
Member

@rossabaker rossabaker Feb 14, 2019

I think it's still a better solution than reflection.

Copy link
Contributor

@nebtrx nebtrx Feb 14, 2019

I agree too.

BTW, I just find out about this now. @ChristopherDavenport next time please take some time to ask if it would be nice to apply the changes on your own. I had your suggestions almost ready for a couple of days now but I was waiting to get back to my personal computer after being AFK for some days due to personal matters.

Don't misunderstand me, I appreciate your review, suggestions and the work done to push this into master but, imagine if I'd expend more than just a few minutes on that. 😉

Anyways, thanks for everything and, apologies for arriving late to the party.

Copy link
Member Author

@ChristopherDavenport ChristopherDavenport Feb 14, 2019

@nebtrx I apologize if I rushed things. We've been working up to another milestone release and when I did not hear back I pushed forward as I wanted this to make the release.

It was not my intention to undercut your efforts and truly appreciate all the time and thought it took for you to put this together.

I hope this will not discourage you from contributing in the future. I am sorry, and will do my best moving forward to wait for additional feedback.

Copy link
Contributor

@nebtrx nebtrx Feb 14, 2019

Nah, it's okay. I get it. I truly appreciate the work you guys are doing and you can bet I'll be contributing again.

@ChristopherDavenport ChristopherDavenport merged commit 4b92d37 into http4s:master Feb 14, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants