|
History
| Log In
|
|
|
|
[
Permlink
| « Hide
]
Werner Guttmann - 07/Jul/07 05:53 AM
Wouldn't it make more sense to add a <handlers> section for handler definition, and subsequently refer to these handlers elsewhere. Afair, this is how we deal with key generator defs on the JDO side of things, isn't it ?
I'm not familiar with the JDO side of Castor, but having browsed the documentation I agree that this would make the handlers even more reusable.
Have a look at http://castor.org/key-generator.html
 to get an idea how key generators are defined in Castor JDO. If we agree that this is the way to go forward, can you please cater for this in your work ? i've got a first working version.
It works very similar to the JDO key-generator: <mapping> <class name="com.acme.MyClass"> field-handler elements (new) can be defined at mapping level, and are referred to by the (existing) "handler" attribute of the "field" element. The handler attribute already supported FieldHandler classnames and short-hand types like "integer". But if it specifies the name of a field-handler that is declared at mapping level, that field handler is used. Field handlers declared in a field-handler element must be implementing the FieldHandler interface. If parameters are specified with the "param" elements, the class must implement the (new) ConfigurableFieldHandler interface. This interface simply lets you set a Properties object. It's up to the author of the field handler to do something useful with it. As a convenience, I've implemented the ConfigurableFieldHandler interface in AbstractFieldHandler, which means that most existing FieldHandlers can use the configuration right away. This change is fully backward compatible, because the handler attribute still works with the current values (fully qualified classnames and short-hand types such as integer). I'll post the patches as soon as we've solved CASTOR-2041. I'd rather see a patch attached here as CASTOR-2041 might take a while to be resolved. Great work, though, based upon your description. I'll be back with more feedback once you've attached a patch (against SVN trunk).
Some random observations:
About the prefix: do you mean to use a separate namespace? For which elements would you suggest to use it? I'm afraid I don't really understand what you mean...
Thanks for the compliments – wait until you see the patch Supplying the patch before CASTOR-2041 is solved is a bit of a problem. Since I've modified the mapping.xsd, I've also re-generated the mapping sources. On top of that, I had to change a number of other core classes, to let them work with the changed mapping classes. Since the core module now uses the mapping sources that are in svn (the build doesn't yet automatically generate the mapping sources and use them to compile the core against), the build will fail after applying my patch. Solving this issue requires the elimination of the cyclic dependency first, I'm afraid. As I've described in CASTOR-2041, I've got some changes ready for build.xml, that incude the generation of sources in the regular build cycle. Let me know if you want to have my changes. No, my comment about prefices was not related to namespaces at all. In Spring, when you define a resource in your Spring application context, you can use resource prefices such as
classpath://org.exolab.someFile or http://some.server.com/some/location/file I think it would be a good idea to make things a bit more explicit in a mapping file. But that's really a nice to have. > Solving this issue requires the elimination of the cyclic dependency first, I'm afraid.
Not really. One can still generate the classes elsewhere and 'copy' modified over existing code. This is what I did in the past. It's a pain in the arse, but it works. And it will allow us to progress these two issues unrelated. Ok, so the above example would read
<field-handler name="myconfigurabledatehandler" class="classpath:com.acme.ConfigurableDateFieldHandler"> http://static.springframework.org/spring/docs/2.0.x/reference/resources.html#resources-resourceloader Let me first try to supply the patches with the trick you decribe. If that all works out, I'll give this feature a try. Hmm, now that I think a bit more about the classpath prefix; it's a nice feature, but in this context (specifying the class of a field hander), it's actually redundant. A field handler is not just a resource, it will always be a class, hence the attribute name "class". So in this situation, we will never specify any other type of resource than a class.
I think this feature will shine more in other situations. mappingSources.txt is the patch that contains all changes to the generated sources (src/main/java/org/exolab/castor/mapping/xml/). Note that the source generator sets up a package stucture that is a bit different. Most changes are a result of this.
mainChanges.txt is a collection of patches that implement the new feature. They rely on the new generated mapping sources. testcase.txt provides a simple testcase for the new feature. To me resource prefices seam to be a point that needs to be discussed separately and should not been introduced as part of a patch that adds parameterized FieldHandlers. I therefore suggest you to open a new issue for resource prefices if you think this is something to think about Werner.
Yes, Ralf, as this potentially could be applied in many places. I am not too eager to introduce this now, but some of Tom's comments above about how things work between references, fully qualified class names et alias got me thinking (aloud).
I am just about to commit above patches as is ... excluding the changes to the mapping classes (as generated from the XML schema instance, which I had to do myself). Tom, I really appreciate your work, and I think it is of very high standard.
Having said that, I'd like to see the following additions being made in addition to what you've done so far.
Tom, please attach any patches regarding the documentation to any of the three sub-tasks I have just created. I have marked this issue as resolved, as I have committed the code.
These changes are a simplification of the ConfigurableFieldHander interface. One of the methods was actually unnecessary.
Tests are updated, and one extra test added. The also documentation reflects this change. Let me be a bit clearer. My previous comment is related to the patches I attached today to the main task. I've also submitted the requested patches to the 3 subtasks.
Thanks, Tom, Are you in a position to supply me with one patch file only. Just position yourself in src/main/java, and issue an SVN diff. Capture the output to a file, and attach it here. That should do it
Tom, can I please ask you to attach the new patches (holding the simplifications) to the sub-task I just created. Thanks in advance.
Ok, I've moved the patch files to the new sub task.
However it's not possible to create a single patch file for each of the subtasks. It all or nothing. I hope this is ok for you. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Major
Add support for configurable FieldHandlers
