DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUGĀ·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=41514>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED ANDĀ·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=41514





------- Additional Comments From [EMAIL PROTECTED]  2007-02-12 09:02 -------
Hi Vincent,

Thanks very much for taking the time to look closely at my patch.

(In reply to comment #26)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > Adrian, thanks for doing this! I've looked at the patch and have a few
comments
> > > myself:
> > > - I'd suggest to rename "strict-configuration" to "validate-configuration"
(just
> > > a personal preference).
> > 
> > The configuration will still be validated regardless of the setting of this
> > variable.  Its just how the error is handled that makes the difference.  If
> > "strict-configuration" is true then FOP will immediately throw an exception 
> > and
> > processing will terminate.  If "strict-configuration" is set to false then 
> > FOP
> > will log the error and attempt to continue parsing the configuration (if
> > possible/meaningful).
> > 
> > > - the name of the variable "strictFO" in 
> > > FopFactory.configure(Configuration)
> > > seems wrong. There's nothing "FO" specific there. Furthermore, some "if
> > > (strictFO)" should actually be "if (strictConfig)", right?
> > 
> > I think you may have been looking at an older patch.  The variable
> > "strictValidation" is as before and the new variable is called
> > "strictUserConfigValidation".
> 
> No, Jeremias is right actually. There are 'if (strictFO)' statements to test 
> if  
> an exception has to be thrown. That should be 'if 
> (validateUserConfigStrictly())'. I should have better looked.

Darn it..  I didn't think Jeremias was refering to the configure method.  I
missed this, thanks.

> Also, in the new code a HyphenationTreeResolver is no longer created and 
> assigned to the hyphResolver field. That should inevitably lead to 
> a NullPointerException later, however the hyphenation junit tests pass. 
> Strange. 
> Are you sure of what you're doing?

Thanks for checking this, I made a mistake refactoring here :-(.

> One other thing: when getting the default-page-settings parameter it's not 
> necessary to test if pageConfig is null; it will never be. See the javadoc of 
> getChild.

I've read the javadoc and have simplified/removed this unnecessary check now.

> >  
> > > Adrian, would you please install the CheckStyle plug-in in your IDE? There
are a
> > > few nits about the Java style in your patch. Checkstyle will help you find
them.
> 
> Something checkstyle doesn't seem to be catching: we usually don't put spaces 
> inside brackets: if (test) instead of if ( test ).

Yes I have noticed this recently and believe all the code I've added adheres to
this now.

> > > 
> > > Get well quickly, Adrian!
> > 
> > After a weekend in bed am feeling much better today thanks.  I had installed
> > checksytle but not enabled it! ;-(  I will recreate the patch this morning.
> 
> Hope you feel better now!
> 
> Sorry, I'll ask you to create yet another patch, but that should really be 
> the 
> last one!

Its ok, I am learning all the bumps for fop patch submissions - hopefully should
be smoother for you guys next time :-).

Kind regards,

Adrian.

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

Reply via email to