dev@javaserverfaces.java.net

Re: Seeking Review [1472-CCRequiredAttribute]

From: Ed Burns <Ed.Burns_at_Sun.COM>
Date: Tue, 05 Jan 2010 20:13:37 -0800

>>>>> On Mon, 04 Jan 2010 17:10:35 -0500, Andy Schwartz <andy.g.schwartz_at_gmail.com> said:

AS> On Mon, Jan 4, 2010 at 4:11 PM, Jim Driscoll <Jim.Driscoll_at_sun.com> wrote:
>>
>> 2) I don't know offhand what Exception I'd pick to throw, but I'm sure that
>> IOException isn't the one I'd choose.  IllegalStateException is probably
>> better... though there's probably a better one still that I haven't thought
>> of.

AS> Maybe javax.faces.view.facelets.TagException?

That's what I did.

AS> I noticed that the check for the presence of the required facet is
AS> checking the attribute map (calling attrs.containsKey(key)) instead of
AS> checking the facet map (by calling getFacet(key)). Is this
AS> intentional?

Great catch! Thanks. I've added test coverage for that too.

AS> Oh, also... It looks like it is possible that the previous value for
AS> the "required" variable may survive across loop iterations. That is,
AS> say the first attribute has required="true" and the value for the
AS> attribute is properly specified. We move onto the next iteration of
AS> the loop. The next attribute does not specify required metadata.
AS> Won't the "required" variable still be true when we hit the "if
AS> (required)" block?

Again, another great catch.

Ed

-- 
| ed.burns_at_sun.com  | office: 408 884 9519 OR x31640
| homepage:         | http://ridingthecrest.com/