Steve,

Any additional input before I upload a new patch?

-Rob


On 09/03/2009 10:28 PM, Rob Hasselbaum wrote:

Hi Steve,

Thanks for the feedback. I am working on moving the patch to trunk as you requested. I'll update the bug when that's done. My comments on the rest of your feedback are inline below.

On 09/03/2009 04:39 PM, Steve Ebersole wrote:

1) Personally, I don't like the attribute names sql-get and sql-set.
When I think through trying to describe and explain this feature to
people, the terms "wrap" and "unwrap" keep coming to my head as being
descriptive, relevant and natural.  It was really the "naturalness"
aspect that got me with sql-get and sql-set.  Other terms I could think
of included encode/decodeAny other suggestions?  

I went with "sql-set" and "sql-get" because they are consistent with the existing "sql-type" attribute and make sense to me, but I'm not married to the terms. Wrap and unwrap sound good, too. Bear in mind that the expressions need not be function calls, which wrap and unwrap might suggest. The functional tests do math, for instance.

2) Can anyone foresee a valid use-case for allowing one of these to be
defined w/o the other?  The only thing I came up with was for immutable
properties.  So something like <column name="xyz" unwrap="decode(xyz)"
insert="false" update="false"/>, which can also be defined using a
formula like <formula>decode(xyz)</formula>.  The reason I ask is that
if they should always be used together then maybe it is better to
enforce that in the DTD and/or binder

I couldn't think of a use case for a one-way conversion either. In fact, you could end up reading out a different value than the one you put in if you're not careful. But I'd regret adding an artificial restriction if someone came up with a legitimate use case next year.

3) You renamed the getTemplate method on Selectable to
getGetterTemplate.  Everything in the o.h.mapping package is an SPI that
is used by many other libraries.  We need to be very careful about
changing stuff in here.  I am not sure if folks bind to this particular
method.  But since this is just a cosmetic change, I think it should be
reverted.

Sorry, this was an oversight. At first, I thought I was going to need to store a setter template in addition to a getter template in the Column class,  It turned out that I didn't need it, but I forgot to revert the name of the getter template. I'll fix that.

Still not done looking through the whole patch.  Will try to finish up
tomorrow.

Thanks,
-Rob

_______________________________________________
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev