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