On 19 November 2015 at 10:01, <pbenedict_at_apache.org> wrote:
> My initial impression of PushBuilder:
>
> * I find it a surprising choice to make it a default method that always
> returns an instance. This is very confusing, IMO, because the
> connection may not support the feature -- which clearly begs the
> question why (1) instantiate anything and (2) return an object that
> does nothing?
>
I believe that feedback has already been received that there should be an
isPushSupported method and then either null or ISE can be returned if it is
not. I'm OK with either approach.
> * Is it wise for a spec to provide server implementation classes? I
> don't think this precedent should be set. That was clearly the purpose
> for introducing NoOpPushBuilder.
>
Hmmm a bit late for the servlet spec to avoid implementation classes :)
But the approach above avoids the need for the noop.
>
> * The docs talk about "If the current connection does not support
> server push", but there is no way to query for that fact.
>
I definitely support the isPushSupported method
> * The methods like "etag" and "path" are truly setters, not operations.
> I think prefixing them with "set" should be considered... and returning
> "void" to keep the bean spec.
>
It is the builder pattern that they are following. But I can live with
either style.
cheers
--
Greg Wilkins <gregw@webtide.com> CTO http://webtide.com