Re: Seeking Review: 21-MethodBinding - Partial

From: Ed Burns <Ed.Burns_at_Sun.COM>
Date: Thu, 11 Nov 2004 07:32:21 -0800

>>>>> On Thu, 11 Nov 2004 10:04:37 -0500, Roger Kitain <Roger.Kitain_at_Sun.COM> said:

RK> Please review the change bundle at:

RK> Note: This is only a partial checkin for ConverterTag/ValidatorTag in api.
RK> The rest of the checkin will be done for some core tags (RI) and TLV
RK> modifications.

1. New Messages

First, I would have preferred to have done the new messages, some of
which appear to have nothing to do with 21-MethodBindings, in a separate
checkin. You can leave it as is now, but in the future, I like to keep
the checkins as tightly focused and cohesive as possible.

Second, all of these must be added to section in the spec PDF

+ public static final String INVALID_EXPRESSION_MESSAGE_ID =

Third, this should be javax.el.INVALID_EXPRESSION. We can do it this
way now since it's only a message key, and a new one at that.

+ public static final String COMPONENT_FROM_TAG_ERROR_MESSAGE_ID =
+ public static final String NOT_NESTED_IN_TYPE_TAG_ERROR_MESSAGE_ID =
+ public static final String NOT_NESTED_IN_FACES_TAG_ERROR_MESSAGE_ID =
+ public static final String CANT_CREATE_CLASS_ID =

2. Error spacing

+ if (converterId != null) {
+ if (converterError != null) {
+ converterError += "or " + converterId;
+ } else {
+ converterError = converterId;
+ }

Don't you need a " " before the "or "?

3. null checking

In the new createConverter(), you say:

+ converterIdVal = (String) idBinding.getValue(context);
+ }
+ converter =
+ context.getApplication().createConverter(converterIdVal);

If idBinding.getValue() return null, converterIdVal will be null, and
you'd get an NPE. Is that what we want? It may be, I just wanted your
opinion on it. I'm ok leaving it as is, since I suspect that's how it
was to begin with.

4. I18N messages. Please add these new messages to all the locales.
For non-english, just use the english versions, and our I18N team will
localize them for us.

Make these changes and you have r=edburns to go through the checkin


To unsubscribe, e-mail:
For additional commands, e-mail: