On Thu, Mar 27, 2008 at 2:54 AM, Antonio Petrelli <[EMAIL PROTECTED]> wrote: > Anyway, I took a look at your patch and I'd like that you give me a > hand in building the final patch, since I am currently the only active > developer at the moment and I cannot do it only by myself. > 1) Fix your code by using the Checkstyle rules: > http://svn.apache.org/repos/asf/tiles/maven/trunk/build/tiles_checks.xml > If you are using Eclipse, there is a Checkstyle plugin to check it: > http://eclipse-cs.sourceforge.net/ > 2) Add a JUnit test > 3) Modify the "tiles-test" project to use your definitions factory, > adding a test page and the Selenium test. > 4) Add a wiki page to show how it works: > http://cwiki.apache.org/TILES/
I agree with the above. I bet we could accept the patch without the wiki page, but we certainly do need documentation about how it works. > > Currently it is a separate factory, with the createDefinitions() > > method overridden. > > I think this WildcardDefinitions should be used by the URLDefFactory > > by default. > > Mmm... I am -1 for this, since it adds some extra complexity. I think > that it is simply a matter of configuration in web.xml. I know that > this means that many people can access a feature that they do not know > to have :-) but I think that a good documentation could be enough. I'm torn on that one. The functionality seems useful enough to activate it by default. But I can understand the complexity issue. Is there a "less-complex" way to add it to the base configuration system? Is there a reason a user might want the functionality to *not* be enabled? > > One more issue which needs to be discussed is, that this > > implementations has references to the XWork library. I don't know > > whether it is a problem or not. > > I don't think so, it could be an optional dependency. Anyway, are you > sure that you really need XWork? I notice that you do not use XWork as > it is, but only a pair of classes. Probably, internally, XWork uses > some other "common" classes. Can you take a look? If xwork is needed it's not a problem to include it. It's ASL-licensed. However, I agree with Antonio that I'd hate to include unnecessary dependencies. If there's a simpler way to provide the functionality you're looking for there it would be nice. Thank you very much for contributing! Greg
