-
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
Moved BaseHolder and Source to core ee #11744
Conversation
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'm in 2 minds about this: one the one hand it is nice to reduce code duplication, plus a lot of the changes are just mechanical and not substantive; but on the other hand it is a PITA.
* @param <T> the type of holder | ||
*/ | ||
@ManagedObject("Holder - a container for servlets and the like") | ||
public abstract class ServletContextHolder<T> extends BaseHolder<T> |
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 this name is misleading: it is not holding a ServletContext
. Not sure of what a better name might be? ServletContextScopedHolder
?
return new UnavailableException(message); | ||
} | ||
|
||
@ManagedAttribute(value = "Display Name", readonly = true) |
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.
Can't all the methods from here on down be pushed down into BaseHolder
? They're independent of the servlet spec version, and I don't think its important if they are not used by some of the derived classes.
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.
My feeling is -1 on this change.
IMHO the cost of this rather small code deduplication isn't worth the complexity of this change.
I'll close this soon, but I'll first look through for any worthwhile cleanups in the PR. |
This was a lot of work just to move 2 small classes to core.
We have to evaluate if the reduced duplication is worth the complexity of the split.
Note that I have not yet updated the EE10 code to use these common classes, will be the same changes as made to EE11