-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Converted writeTrailers to a static method #8940
Conversation
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
Show resolved
Hide resolved
* @see Content#copy(Content.Source, Content.Sink, BiPredicate, Callback) | ||
* @see Trailers | ||
*/ | ||
static BiPredicate<Content.Chunk, Callback> asTrailerChunkHandler(Response response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of a mouthful. Can't we just keep it named writeTrailers
?
Or perhaps trailersWriter
to indicate that it returns a writer that will write the trailers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the new name is good:
- It does not write trailers, it copies them from one HttpFields to another, to be written at a later time.
- If it was just writing/setting trailers, then the signature would be a lot different. It has a very specific signature so that it can be used for a very specific purpose: to be a chunk handler for a copy operation.
- It could perhaps just be
asChunkHandler
, so that in future other types of chunk might be handled, but I doubt we will ever do that. Having a name that tells us it is used for trailers will help coders looking for a way to handleTrailers
chunks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handler
is so overloaded...
Taking into account your points, how about Response.trailersCopier(Response)
?
The "as" in front should be dropped as it is not "converting" the response to something else -- I would tolerate "get" as in getTrailersCopier
, but then it creates a new one every time, so newTrailersCopier
, but then dropping the prefix make it nicer:
Content.copy(request, response, Response.trailersCopier(response), callback);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the argument in the copy signature is called chunkHandler
, so the names have to match (and that is your name choice in Content!).
I could rename the arg to chunkTransformer
? but then this isn't transforming it? or chunkProcessor
, but then Processor
is overloaded as well.
But regardless, this PR is just about fixing a TODO in the Response
class. If you want to change the name of the chunkHandler
in the Content
class, then do that in a different PR?
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
Show resolved
Hide resolved
closed as we couldn't agree on a name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fiiine 🥲, just update the javadocs as suggested 😄
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregw more javadoc fixes.
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
Outdated
Show resolved
Hide resolved
…/Response.java Co-authored-by: Simone Bordet <[email protected]>
…/Response.java Co-authored-by: Simone Bordet <[email protected]>
…/Response.java Co-authored-by: Simone Bordet <[email protected]>
…/Response.java Co-authored-by: Simone Bordet <[email protected]>
…/Response.java Co-authored-by: Simone Bordet <[email protected]>
Fixed TODO regarding making writeTrailers a static method.