You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tiles.apache.org by Cserveny Tamas <sm...@dev.hu> on 2008/03/26 21:49:48 UTC
TILES-220: Wildcard (*) in definitions
Hi,
I've implemented a draft solution to the TILES-220 issue. It is
basically an extension of the DefinitionsImpl.
If a definition is registered with * in the name, it is remembered for
later use.
If a definition cannot be found, then these names will be checked,
whether any of them are matching.
If one of them matches, then a new definition will be made, and then
will be remembered, so no more matching needed.
There are some open questions though:
My implementation is basically a solution that can be used without any
modification in tiles.
I think it would be good to include this feature in later versions,
thus it needs to be integrated more tightly into Tiles.
Currently it is a separate factory, with the createDefinitions()
method overridden.
I think this WildcardDefinitions should be used by the URLDefFactory
by default.
This WildcardDefinitions could be a wrapper as well, in case it is
desired/forseen that the Definitions class could change / or other
implementations might be needed. (In this case the wildcard matching
would still be provided)
It is also possible to include this matching in the DefinitionsImpl
itself, but may be that would complicate the DefinitionsImpl too much.
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.
What is your opinion about the topic?
--
Cserveny Tamas
Re: TILES-220: Wildcard (*) in definitions
Posted by Greg Reddin <gr...@gmail.com>.
On Thu, Mar 27, 2008 at 10:11 AM, Antonio Petrelli
<an...@gmail.com> wrote:
>
> XWork has an ASL-compatible license, but not ASL:
> http://www.opensymphony.com/xwork/license.action
> My doubts are about the inclusion of this dependency because it is a new
> dependency, its license is OK.
Interesting, I found it at java.net under the ASL:
https://xwork.dev.java.net/
I guess the opensymphony site is more official though.
Greg
Re: TILES-220: Wildcard (*) in definitions
Posted by Antonio Petrelli <an...@gmail.com>.
2008/3/27, Greg Reddin <gr...@gmail.com>:
>
> On Thu, Mar 27, 2008 at 2:54 AM, Antonio Petrelli
> <an...@gmail.com> wrote:
> > 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.
+1. I'm sorry, I meant: after all of the above, it would be nice if you
write a wiki page to show how to use it.
> > 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?
I don't think so, Cserveny did a good job in finding the right point of the
intervention.
Is there a reason a user might want the functionality to *not* be
> enabled?
The reason is the XWork dependency. If it will be removed, then I think that
it could be used as a default.
If xwork is needed it's not a problem to include it. It's
> ASL-licensed.
XWork has an ASL-compatible license, but not ASL:
http://www.opensymphony.com/xwork/license.action
My doubts are about the inclusion of this dependency because it is a new
dependency, its license is OK.
One more problem is that we cannot include XWork code and strip out the
XWork header. Paul Benedict had a similar issue with Struts and Spring code,
and the Legal team of ASF told him to put both Apache and Spring headers on
top of source code. Anyway, in this case, we can ask the Struts/XWork team
about licensing terms/ source headers.
Antonio
Re: TILES-220: Wildcard (*) in definitions
Posted by Greg Reddin <gr...@gmail.com>.
On Thu, Mar 27, 2008 at 2:54 AM, Antonio Petrelli
<an...@gmail.com> 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
Re: TILES-220: Wildcard (*) in definitions
Posted by Antonio Petrelli <an...@gmail.com>.
2008/3/26, Cserveny Tamas <sm...@dev.hu>:
> I've implemented a draft solution to the TILES-220 issue. It is
> basically an extension of the DefinitionsImpl.
> If a definition is registered with * in the name, it is remembered for
> later use.
> If a definition cannot be found, then these names will be checked,
> whether any of them are matching.
> If one of them matches, then a new definition will be made, and then
> will be remembered, so no more matching needed.
Cool :-)
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/
> My implementation is basically a solution that can be used without any
> modification in tiles.
> I think it would be good to include this feature in later versions,
> thus it needs to be integrated more tightly into Tiles.
If everything goes well, it will be included in Tiles 2.1.0
> 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.
> This WildcardDefinitions could be a wrapper as well, in case it is
> desired/forseen that the Definitions class could change / or other
> implementations might be needed. (In this case the wildcard matching
> would still be provided)
In fact, some time ago we decided to not allow the configuration
(through web.xml parameters) of Definitions class, since it shows an
internal "gut" of the particular implementation of the Definitions
factory. I think that a subclass of UrlDefinitionsFactory as you did
is ok, I created the "createDefinitions" method on purpose :-)
> It is also possible to include this matching in the DefinitionsImpl
> itself, but may be that would complicate the DefinitionsImpl too much.
This is another reason why I don't like to see it as a default.
> 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?
One more thing. I noticed that you called the patch "2.0.5.patch".
Does it mean that you created it starting from the 2.0.5 version? If
yes, please create a patch against the trunk in SVN repository:
http://svn.apache.org/repos/asf/tiles/framework/trunk/
Thanks
Antonio