Jason van Zyl <[EMAIL PROTECTED]> writes:

> On 1/11/02 6:37 PM, "[EMAIL PROTECTED]" <[EMAIL PROTECTED]> wrote:
>
>> dlr         02/01/11 15:37:56
>> 
>> Modified:    src/java/org/apache/stratum/xo Mapper.java
>> Log:
>> o Removed use of basePackage -- it made too many assumptions about how
>> the class hierarchy is structured.  With it, child elements had to be
>> in the same package as their parent.
>
> Why did you do that? That's one of the whole tenets of this class is that
> the simple case works. For a small object model the classes will be in the
> same package. I certainly don't mind the addition of logic to handle what
> you wanted for the pipeline, but you i) didn't discuss anything ii) you
> broke the testbed. What you removed I think is a real convenience, you could
> have added your more sophisticated behavior overtop of what was there.

I agree with that last statement -- I said approximately the same on
IRC before reading this mail:

[01:42:00] <dlr_h> jason: WRT Mapper, you can now either pass in a
class name to map(), or null to have the Mapper examine the descriptor
for a className element or attribute
[01:43:16] <dlr_h> Using the package of the enclosing class is an
unreasonable assumption for the default behavior -- Scarab SourceCast
integration broke it immediately.  It might be okay as a tertiary
behavior, for use if the first two checks fail.

So having now gone over this evening's changes, I think that the
className attribute should be looked for first, then if that doesn't
exist, the simple case where nested elements are in the same package
as the parent should be tried.  I need to take another look, but I
think this is the reverse of the order specified in your changes
tonight.  I think the check for the className should come first,
because if users go to the trouble to specify the className attribute,
that's probably the value that they want for className.  As you say
above, the simple case where no className is specified will still work
like a charm.

>> o Added elementClassName() method for use in discovering the class
>> name of a nested element (and allowing the removal the the "same
>> package" assumption).
>
> A big fat -1, the torque object model can be rather complicated and it fits
> perfectly fine in a single package.

But it doesn't work well when integrating full Turbine components into
a larger system where some percentage of their pipeline need to be
replaced.  I think we're on the same page (as in the previous
comment), so figure you're just objecting to having the simple
behavior removed (please speak up if otherwise).

>> o Renamed elementToClassName() to makeMethodName().  As its
>> assumptions about package structure didn't hold, it's now used only to
>> generate the name of the method used to add objects to a parent bean.
>
> The assumption definitely holds as one simple, one moderately complicated
> model (Torque), and one pretty complicated model (Maven) all worked under
> this assumption and as the tests showed the assumption works fine. Like I
> said additional complexity is fine, but there's no reason to remove handling
> for simple and commons cases.

The assumption failed when used with the kind of model used in
component integration (Scarab) into a large system (SourceCast).

> I will put the behaviour back as I've already made changes to make the class
> work with InputStreams so it can deal with resources in the filesystem and
> the classpath.

You regressed the classpath loading I'd already added.

Dan

--
To unsubscribe, e-mail:   <mailto:[EMAIL PROTECTED]>
For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>

Reply via email to