Add StaplerResponseWrapper #73
Conversation
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
needed (e.g. capturing output by overriding getWriter())
|
🐝 - pretty straightforward |
| public abstract class StaplerResponseWrapper implements StaplerResponse { | ||
| private final StaplerResponse wrapped; | ||
|
|
||
| public StaplerResponseWrapper(StaplerResponse wrapped) { |
There was a problem hiding this comment.
🐜 but not a bad idea to put a @NotNull to enforce non-nullity of the wrapped response
There was a problem hiding this comment.
Maybe, but you might need to override getWrapped() to get a StaplerResponse by some other means (I've had to do this in the HttpServletResponseWrapper world before), and having a null wrapped member variable is valid in that case.
There was a problem hiding this comment.
@kzantow Wouldn't it make more sense to just extend StaplerResponse in that case? You're not really using the wrapper then.
There was a problem hiding this comment.
You'd still need to forward all the calls to some StaplerResponse somewhere. Let's say you wanted to @Inject StaplerResponse into something with a "larger scope" in a DI system, you couldn't set the current response and you'd have to provide a wrapper, for example, to look it up from a ThreadLocal variable every time.
There was a problem hiding this comment.
Hmm... okay I can see that case. On considering again, I think getWrapped() would be the right place to put NotNull, not on initialization.
|
🐝 |
|
@svanoort 's comment addressed |
|
🐝 |
|
LGTM |
... for wrapping functionality downstream if needed (e.g. capturing output by overriding getWriter()).
@reviewbybees esp. @jglick