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

Message cleanup #2234

Merged
merged 6 commits into from Nov 5, 2018

Conversation

Projects
None yet
3 participants
@aeons
Member

aeons commented Nov 1, 2018

This builds on top of #2210, and should not be merged before that.

This PR does the following:

  • Remove deprecated Effect{Message,Request,Response}Syntax traits and associated objects
  • Remove {Message,Request,Response}Ops and put the removed methods, sans unneeded implicit parameters, directly in the classes
  • Deprecate replaceAllHeaders, pointing to withHeaders instead. The other direction of this was done in 0.10, apparently. Currently, all withX methods replace the X. For headers, we have withHeaders that replaces and putHeaders that adds.
  • Deprecate withType, which takes a MediaType and just wraps it in a Content-Type
  • Deprecate withContentTypeOption, recommending to just do the Option.fold in user code
  • Add withoutAttribute and withoutTrailerHeaders to complement the with variants
  • Change some defs into lazy vals
  • filterHeaders' scaladoc comment said that it would remove all headers that the predicate was true for, while doing the opposite. I have changed the comment to be correct and changed withoutContentType which depended on the behaviour described in the comment.

Many of the methods in the *Ops traits were final. I have kept that where it made sense, but I'm not sure it's needed. Can anyone clarify?

Closes #2224

@aeons

This comment has been minimized.

Member

aeons commented Nov 1, 2018

I'll rebase onto master once #2210 is merged. The diff here will also be quite messy until that happens

@aeons aeons added this to the 0.20.0 milestone Nov 1, 2018

aeons added some commits Nov 1, 2018

Remove Message/Request/ResponseOps
Put the removed methods in Message/Request/Response
Remove unused implicit parameters
Deprecate and rename some methods to align naming

@aeons aeons force-pushed the aeons:message-cleanup branch from 982ce19 to 6f05ec8 Nov 2, 2018

@aeons aeons changed the title from WIP: Message cleanup to Message cleanup Nov 2, 2018

@aeons aeons removed the don't merge yet label Nov 2, 2018

@aeons

This comment has been minimized.

Member

aeons commented Nov 2, 2018

I think the appveyor failure is unrelated.

@rossabaker

This is nice. I mostly question the laziness. Would like to hear from others.

def withHeaders(headers: Headers): Self =
change(headers = headers)
def withHeaders(headers: Header*): Self =

This comment has been minimized.

@rossabaker

rossabaker Nov 2, 2018

Member

Is there a better naming idiom we could adopt here and avoid the overload? I've grown more hostile to those, but don't have a naming suggestion.

This comment has been minimized.

@aeons

aeons Nov 5, 2018

Member

I don't have a better idea.

Show resolved Hide resolved core/src/main/scala/org/http4s/Message.scala Outdated
Show resolved Hide resolved core/src/main/scala/org/http4s/Message.scala Outdated
Show resolved Hide resolved core/src/main/scala/org/http4s/Message.scala Outdated
Show resolved Hide resolved core/src/main/scala/org/http4s/Message.scala
* Attempt to decode the [[Request]] and, if successful, execute the continuation to get a [[Response]].
* If decoding fails, an `UnprocessableEntity` [[Response]] is generated.
*/
final def decode[A](

This comment has been minimized.

@rossabaker

rossabaker Nov 2, 2018

Member

I'm skeptical of the value of this and decodeStrict, but more people use them than I realize. I don't know that I want to fight the deprecation battle for something so common, but I dislike having multiple ways to do it.

This comment has been minimized.

@aeons

aeons Nov 5, 2018

Member

I would be 👍 on deprecating it. I guess we can look at reports for it for the milestones as a gauge for how much it's used.

This comment has been minimized.

@rossabaker

rossabaker Nov 5, 2018

Member

Deprecation + scalafix is going to be a powerful combination for us for things that can be translated directly. It would be nice if the two start going together once the scalafix lands. I'm comfortable taking this on in a subsequent PR.

Show resolved Hide resolved core/src/main/scala/org/http4s/Message.scala
@aeons

This comment has been minimized.

Member

aeons commented Nov 5, 2018

Is there a point to having final def vs. def in a subclass of a sealed trait. Does it hint anything to the compiler about inlining?

@rossabaker

This comment has been minimized.

Member

rossabaker commented Nov 5, 2018

It can theoretically help with static optimizations, but the JVM is pretty smart about what's actually overridden. I would expect it to not make a practical difference at runtime, and it's a question of signaling intent. Here, I don't think it's a huge deal either way.

@rossabaker

👍

@aeons

This comment has been minimized.

Member

aeons commented Nov 5, 2018

Alright. To be consistent I've made all of them non-final. :)

@ChristopherDavenport

👍 after resolved

@rossabaker rossabaker merged commit 56be1ac into http4s:master Nov 5, 2018

1 of 2 checks passed

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

@aeons aeons deleted the aeons:message-cleanup branch Nov 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment