You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by Eelco Hillenius <ee...@gmail.com> on 2007/08/13 20:38:28 UTC
VOTE: setRequired final or not?
Igor and I are still in deadlock. We need a vote.
I am arguing for removing final from setRequired so that I can do this
in DateField:
public FormComponent setRequired(boolean required)
{
super.setRequired(required);
dateField.setRequired(required);
return this;
}
That way you can just call setRequired on DateField and it will work.
A caveat is that IF I want isRequired on the DateField to always work
(this is not relevant for form processing, but maybe for things like
adding behaviors etc), I should make isRequired final:
public final boolean isRequired()
{
// prevent clients from overriding
return super.isRequired();
}
Igor argues against this as he thinks the API hole is too dangerous.
He was doing this:
protected DateTextField newDateTextField(PropertyModel dateFieldModel)
{
return new DateTextField("date", dateFieldModel, new StyleDateConverter(true))
{
public boolean isRequired()
{
return DateField.this.isRequired();
}
};
}
This works when you override DateField's isRequired method (which I
guess is a possibility), though the disadvantage of that is that you
have to force your users this contract. So annonymous classes won't
work and there is no way you can enforce this to clients (as you can't
check that they did override the isRequired method appropriately). And
things like
return DateTextField.forShortStyle("date", dateFieldModel);
won't work either.
A third alternative is to override e.g. onAttach:
protected void onAttach()
{
super.onAttach();
dateField.setRequired(isRequired());
}
this leaves a hole when there is a difference between the value of
isRequired during onAttach and when rendering, though I guess this way
we have no API changes at all.
We spent almost a whole day arguing back and forth about the proper
meaning of checkRequired. We left that discussion in a deadlock, and I
removed final from setRequired instead, as I believe it is more
precise to override this method and distribute to children than doing
this in onAttach, and it doesn't leave a hole (though that hole is
arguably not very large) either.
Can you please vote:
[ ] please put final back to setRequired as I think the API hole is
too big and doing it in onAttach or forcing sub components to override
isRequired is good enough for me
[ ] keep setRequired non final so that components can decide to do
something useful when it is called, even though implementators have to
think about what to do with isRequired
Cheers,
Eelco
Re: VOTE: setRequired final or not?
Posted by Eelco Hillenius <ee...@gmail.com>.
On 8/15/07, Matej Knopp <ma...@gmail.com> wrote:
> We never override isRequired in Wicket. So we keep the contract of
> setRequired. But we shouldn't prevent users to override it. If user
> wants to break the set contract, it's ok. I don't see anything bad
> about it.
But do you see anything bad about users overriding setRequired? Which
is why this thread was started...
Eelco
Re: VOTE: setRequired final or not?
Posted by Johan Compagner <jc...@gmail.com>.
thats why i said for internal API protection.
public api shouldn't change, but i guess thats not always what we can
guarantee
johan
On 8/17/07, Igor Vaynberg <ig...@gmail.com> wrote:
>
> On 8/17/07, Johan Compagner <jc...@gmail.com> wrote:
> >
> > I think it is most of the time the responsibility of the developer
> itself.
> > If i want to override then i want to override
>
>
> but if that method is not meant to be overridable then the chances are it
> is
> going to change in the next release and you have to rewrite your code.
> this
> might be ok for method where you call super and then add something, but
> for
> methods where you replace functionality it can be a headache. making
> something overridable is pretty much a contract saying: the scope and
> functionality of this method wont change. most people dont think about
> that,
> they just make it nonfinal by default and later change it at a whim. hell
> some people make everything public....
>
> -igor
>
Re: VOTE: setRequired final or not?
Posted by Igor Vaynberg <ig...@gmail.com>.
On 8/17/07, Johan Compagner <jc...@gmail.com> wrote:
>
> I think it is most of the time the responsibility of the developer itself.
> If i want to override then i want to override
but if that method is not meant to be overridable then the chances are it is
going to change in the next release and you have to rewrite your code. this
might be ok for method where you call super and then add something, but for
methods where you replace functionality it can be a headache. making
something overridable is pretty much a contract saying: the scope and
functionality of this method wont change. most people dont think about that,
they just make it nonfinal by default and later change it at a whim. hell
some people make everything public....
-igor
Re: VOTE: setRequired final or not?
Posted by Johan Compagner <jc...@gmail.com>.
>
>
>
> imho this shouldve been the default java convention. it takes more thought
> and analysis to design something to be overridable.
don't know about that, i think i would have hit my head against the wall way
way more often
then i do now. Sometimes i just HAVE to override behavior of swing
components for example
now they have pieces that are packages scope (textformatters for example for
the jformatted textfield)
thats HORRIBLE. The problem with automatic everything final that nobody
thinks about it
and in the end you have something that can only be used by copy pasting
whole classes.
The shit thing is that that can't be done for example with the
jformattedtextfield because thats not
1 class thats whole packages...
I think it is most of the time the responsibility of the developer itself.
If i want to override then i want to override
where final in my eyes is good for is 2 things: when we depricate methods,
make the final but more
if we don't really want it to be public api but for this we should have
another keyword...
johan
Re: VOTE: setRequired final or not?
Posted by Igor Vaynberg <ig...@gmail.com>.
On 8/16/07, Kent Tong <ke...@cpttm.org.mo> wrote:
>
> Matej Knopp <ma...@...> writes:
>
> >
> > We never override isRequired in Wicket. So we keep the contract of
> > setRequired. But we shouldn't prevent users to override it. If user
> > wants to break the set contract, it's ok. I don't see anything bad
> > about it.
>
> Personally I also don't see anything wrong about it. Just that from
> the core Wicket code it seems the convention is to make methods final
> by default and make them non-final only when necessary.
imho this shouldve been the default java convention. it takes more thought
and analysis to design something to be overridable.
-igor
Re: VOTE: setRequired final or not?
Posted by Kent Tong <ke...@cpttm.org.mo>.
Matej Knopp <ma...@...> writes:
>
> We never override isRequired in Wicket. So we keep the contract of
> setRequired. But we shouldn't prevent users to override it. If user
> wants to break the set contract, it's ok. I don't see anything bad
> about it.
Personally I also don't see anything wrong about it. Just that from
the core Wicket code it seems the convention is to make methods final
by default and make them non-final only when necessary.
Re: VOTE: setRequired final or not?
Posted by Matej Knopp <ma...@gmail.com>.
We never override isRequired in Wicket. So we keep the contract of
setRequired. But we shouldn't prevent users to override it. If user
wants to break the set contract, it's ok. I don't see anything bad
about it.
-Matej
On 8/15/07, Johan Compagner <jc...@gmail.com> wrote:
> the problem is that is* should just be overridable, because can we do
> setvisible in onattach? dont think so because that makes a new
> version. isVis and isEnabled can change between request of a
> component, thats why we have to be able to override it. In the end it
> is always the developers responsibility..
>
> On 8/15/07, Eelco Hillenius <ee...@gmail.com> wrote:
> > On 8/15/07, Igor Vaynberg <ig...@gmail.com> wrote:
> > > if we make is* final that means you can no long pull information which
> > will
> > > lead to ugly code. components that have dynamic * attribute will have to
> > > override onattach/onbeforerender and push state in there - which is much
> > > uglier then simply overriding is*.
> >
> > Though obviously the ugly other side to it is that it breaks the
> > contract of set*. So we now have the situation where you don't want
> > people to override set* because that doesn't give enough guarantees as
> > is* might be overriden. Kind of the inverse world if you ask me.
> > However, I agree that in general having is* non-final is very
> > convenient. I do wonder though how often you would actually use it. I
> > have one case where I use it myself, but that's kind of a hack, and
> > something I can solve in other ways.
> >
> > Eelco
> >
>
Re: VOTE: setRequired final or not?
Posted by Kent Tong <ke...@cpttm.org.mo>.
Johan Compagner <jc...@...> writes:
> the problem is that is* should just be overridable, because can we do
> setvisible in onattach? dont think so because that makes a new
> version. isVis and isEnabled can change between request of a
> component, thats why we have to be able to override it. In the end it
> is always the developers responsibility..
It shows that the VISIBLE flag is only one of the factors determining
the ultimate visibility:
Component c = ...;
c.setVisible(true); //Just one factor
//Don't know if it is truly visible that because it may
//depend on other factors
//assertTrue(c.isVisible());
c.setVisible(false);
assertFalse(c.isVisible()); //This must be the case
Therefore the VISIBLE flag is still used and is not a design
flaw. So it makes sense to keep isVisible() non-final. But the same
thing can't be said for setRequired() and isRequired() because we
can't find a use case where the ultimate result depends on two or
more factors.
Re: VOTE: setRequired final or not?
Posted by Johan Compagner <jc...@gmail.com>.
the problem is that is* should just be overridable, because can we do
setvisible in onattach? dont think so because that makes a new
version. isVis and isEnabled can change between request of a
component, thats why we have to be able to override it. In the end it
is always the developers responsibility..
On 8/15/07, Eelco Hillenius <ee...@gmail.com> wrote:
> On 8/15/07, Igor Vaynberg <ig...@gmail.com> wrote:
> > if we make is* final that means you can no long pull information which
> will
> > lead to ugly code. components that have dynamic * attribute will have to
> > override onattach/onbeforerender and push state in there - which is much
> > uglier then simply overriding is*.
>
> Though obviously the ugly other side to it is that it breaks the
> contract of set*. So we now have the situation where you don't want
> people to override set* because that doesn't give enough guarantees as
> is* might be overriden. Kind of the inverse world if you ask me.
> However, I agree that in general having is* non-final is very
> convenient. I do wonder though how often you would actually use it. I
> have one case where I use it myself, but that's kind of a hack, and
> something I can solve in other ways.
>
> Eelco
>
Re: VOTE: setRequired final or not?
Posted by Eelco Hillenius <ee...@gmail.com>.
On 8/15/07, Igor Vaynberg <ig...@gmail.com> wrote:
> if we make is* final that means you can no long pull information which will
> lead to ugly code. components that have dynamic * attribute will have to
> override onattach/onbeforerender and push state in there - which is much
> uglier then simply overriding is*.
Though obviously the ugly other side to it is that it breaks the
contract of set*. So we now have the situation where you don't want
people to override set* because that doesn't give enough guarantees as
is* might be overriden. Kind of the inverse world if you ask me.
However, I agree that in general having is* non-final is very
convenient. I do wonder though how often you would actually use it. I
have one case where I use it myself, but that's kind of a hack, and
something I can solve in other ways.
Eelco
Re: VOTE: setRequired final or not?
Posted by Igor Vaynberg <ig...@gmail.com>.
if we make is* final that means you can no long pull information which will
lead to ugly code. components that have dynamic * attribute will have to
override onattach/onbeforerender and push state in there - which is much
uglier then simply overriding is*.
-igor
On 8/14/07, Eelco Hillenius <ee...@gmail.com> wrote:
>
> On 8/14/07, Kent Tong <ke...@cpttm.org.mo> wrote:
> > Eelco Hillenius <ee...@...> writes:
> >
> > > > Can't you have an onSetRequired() callback? Or would that be even
> nastier?
> >
> > I thought you had gone through this issue?
>
> Hey, Al wrote that line, not me :)
>
> If a child class provides
> > a onSetRequired() but a grand child wants to override it, it still
> > has to override it. Then why not just let the child and grand child
> > override setRequired()?
> > I think onXXX() callbacks only make sense when the effect can't be
> > achieved with simple method override, eg, cases like:
> >
> > void foo() {
> > do something;
> > onXXX();
> > do something;
> > onYYY();
> > do something;
> > }
>
> I agree. In favor of onXX though is that it would be easier to
> recognize that it is there for overriding and that users don't have to
> call super (though the disadvantage of that can be seen in our
> struggles with onAttach etc).
>
> > > That would be exactly what I want, though bloats our already not tiny
> > > API, but the main problem then still is that users can override
> > > isRequired.
> >
> > Why not make isRequired() final?
>
> I think I would be ok with that. Though a similar case, isVisible, is
> just to convenient to make final. So I can imagine there will be users
> who rather override isVisible. Dunno, we could vote on it I guess.
>
> Eelco
>
Re: VOTE: setRequired final or not?
Posted by Eelco Hillenius <ee...@gmail.com>.
On 8/14/07, Kent Tong <ke...@cpttm.org.mo> wrote:
> Eelco Hillenius <ee...@...> writes:
>
> > > Can't you have an onSetRequired() callback? Or would that be even nastier?
>
> I thought you had gone through this issue?
Hey, Al wrote that line, not me :)
If a child class provides
> a onSetRequired() but a grand child wants to override it, it still
> has to override it. Then why not just let the child and grand child
> override setRequired()?
> I think onXXX() callbacks only make sense when the effect can't be
> achieved with simple method override, eg, cases like:
>
> void foo() {
> do something;
> onXXX();
> do something;
> onYYY();
> do something;
> }
I agree. In favor of onXX though is that it would be easier to
recognize that it is there for overriding and that users don't have to
call super (though the disadvantage of that can be seen in our
struggles with onAttach etc).
> > That would be exactly what I want, though bloats our already not tiny
> > API, but the main problem then still is that users can override
> > isRequired.
>
> Why not make isRequired() final?
I think I would be ok with that. Though a similar case, isVisible, is
just to convenient to make final. So I can imagine there will be users
who rather override isVisible. Dunno, we could vote on it I guess.
Eelco
Re: VOTE: setRequired final or not?
Posted by Kent Tong <ke...@cpttm.org.mo>.
Eelco Hillenius <ee...@...> writes:
> > Can't you have an onSetRequired() callback? Or would that be even nastier?
I thought you had gone through this issue? If a child class provides
a onSetRequired() but a grand child wants to override it, it still
has to override it. Then why not just let the child and grand child
override setRequired()?
I think onXXX() callbacks only make sense when the effect can't be
achieved with simple method override, eg, cases like:
void foo() {
do something;
onXXX();
do something;
onYYY();
do something;
}
> That would be exactly what I want, though bloats our already not tiny
> API, but the main problem then still is that users can override
> isRequired.
Why not make isRequired() final?
Re: VOTE: setRequired final or not?
Posted by Eelco Hillenius <ee...@gmail.com>.
> Can't you have an onSetRequired() callback? Or would that be even nastier?
That would be exactly what I want, though bloats our already not tiny
API, but the main problem then still is that users can override
isRequired.
Eelco
Re: VOTE: setRequired final or not?
Posted by Al Maw <wi...@almaw.com>.
Kent Tong wrote:
> Eelco Hillenius <ee...@...> writes:
>
>> Well nevermind. I'm actually ok with doing this in onAttach. I rolled
>> back the change.
>
> For what it's worth, I think having isRequired() returns a value that
> is different from the REQUIRED flag is a serious design flaw. It
> simply means the REQUIRED flag is unused in this case and is there
> only to confuse people.
>
> On the other hand, should setRequired() be forbidden from doing anything
> extra in addition to setting the REQUIRED flag? The answer should be no
> (as shown in the case of FormComponentPanel). Allowing subclasses to do
> something extra does not necessarily violate any contract established.
> The subclass just has to be careful when overriding setRequired(). This
> is always the case when overriding any method.
>
> Granted, there may be existing code that is abusing isRequired() to make
> the thing work. This may be a reason for not introducing the change right
> now, but it shouldn't be reason for not going toward that goal in an
> orderly fashion.
Yeah, I don't like it either.
Can't you have an onSetRequired() callback? Or would that be even nastier?
Al
Re: VOTE: setRequired final or not?
Posted by Kent Tong <ke...@cpttm.org.mo>.
Eelco Hillenius <ee...@...> writes:
> Well nevermind. I'm actually ok with doing this in onAttach. I rolled
> back the change.
For what it's worth, I think having isRequired() returns a value that
is different from the REQUIRED flag is a serious design flaw. It
simply means the REQUIRED flag is unused in this case and is there
only to confuse people.
On the other hand, should setRequired() be forbidden from doing anything
extra in addition to setting the REQUIRED flag? The answer should be no
(as shown in the case of FormComponentPanel). Allowing subclasses to do
something extra does not necessarily violate any contract established.
The subclass just has to be careful when overriding setRequired(). This
is always the case when overriding any method.
Granted, there may be existing code that is abusing isRequired() to make
the thing work. This may be a reason for not introducing the change right
now, but it shouldn't be reason for not going toward that goal in an
orderly fashion.
Re: VOTE: setRequired final or not?
Posted by Eelco Hillenius <ee...@gmail.com>.
Well nevermind. I'm actually ok with doing this in onAttach. I rolled
back the change.
Eelco
On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
> Let's get your votes! :)
>
> Eelco
>
Re: VOTE: setRequired final or not?
Posted by Eelco Hillenius <ee...@gmail.com>.
Let's get your votes! :)
Eelco
Re: VOTE: setRequired final or not?
Posted by Igor Vaynberg <ig...@gmail.com>.
On 8/13/07, Eelco Hillenius <ee...@gmail.com> wrote:
>
> Igor and I are still in deadlock. We need a vote.
>
> I am arguing for removing final from setRequired so that I can do this
> in DateField:
>
> public FormComponent setRequired(boolean required)
> {
> super.setRequired(required);
> dateField.setRequired(required);
> return this;
> }
>
> That way you can just call setRequired on DateField and it will work.
> A caveat is that IF I want isRequired on the DateField to always work
> (this is not relevant for form processing, but maybe for things like
> adding behaviors etc), I should make isRequired final:
>
> public final boolean isRequired()
> {
> // prevent clients from overriding
> return super.isRequired();
> }
>
> Igor argues against this as he thinks the API hole is too dangerous.
it is necessary to always sync isRequired with setRequired(). it is the same
kind of contract as equals and hashcode. sure you can implement just equals,
and that will work. but you can later easily get in trouble because you did
not implement hashcode.
He was doing this:
>
> protected DateTextField newDateTextField(PropertyModel
> dateFieldModel)
> {
> return new DateTextField("date", dateFieldModel, new
> StyleDateConverter(true))
> {
> public boolean isRequired()
> {
> return DateField.this.isRequired();
> }
> };
> }
>
> This works when you override DateField's isRequired method (which I
> guess is a possibility), though the disadvantage of that is that you
> have to force your users this contract. So annonymous classes won't
> work and there is no way you can enforce this to clients (as you can't
> check that they did override the isRequired method appropriately). And
> things like
>
> return DateTextField.forShortStyle("date", dateFieldModel);
>
> won't work either.
notice that this is only an inconvinience for factory methods that generate
children inside formcomponentpanels. which has a much much much narrower
scope the formcomponent.setrequried nonfinal change.
A third alternative is to override e.g. onAttach:
>
> protected void onAttach()
> {
> super.onAttach();
> dateField.setRequired(isRequired());
> }
>
> this leaves a hole when there is a difference between the value of
> isRequired during onAttach and when rendering, though I guess this way
> we have no API changes at all.
you can also do this in onbeforerender().
-igor
We spent almost a whole day arguing back and forth about the proper
> meaning of checkRequired. We left that discussion in a deadlock, and I
> removed final from setRequired instead, as I believe it is more
> precise to override this method and distribute to children than doing
> this in onAttach, and it doesn't leave a hole (though that hole is
> arguably not very large) either.
>
> Can you please vote:
>
> [ ] please put final back to setRequired as I think the API hole is
> too big and doing it in onAttach or forcing sub components to override
> isRequired is good enough for me
> [ ] keep setRequired non final so that components can decide to do
> something useful when it is called, even though implementators have to
> think about what to do with isRequired
>
>
> Cheers,
>
> Eelco
>