Comments inline below.
Simon
Jean-Sebastien Delfino wrote:
[snip]
Simon Nash wrote:
1. BindingImpl is in org.apache.tuscany.assembly.impl but is
exposed as a subclassing extension point for binding extensions.
I removed BindingImpl as it is not a subclassing extension point. It was
there as a base class for SCABindingImpl, but several of us seem to have
taken it as a subclassing extension point, so it'll be cleaner to remove
it to avoid propagating that pattern.
Agreed, and I like the new implementation.
2. AbstractInvocationHandler and MessageImpl are in the
org.apache.tuscany.invocation interface package. The first
of these is intended for suclassing, so could be considered
an SPI, but the second is not.
I don't think that AbstractInvocationHandler should be an SPI, I suggest
to move to it to the core module. I am not sure why
Axis2CallbackInvocationHandler extends this class, as I don't really
understand the requirement to implement a Java Proxy Invocation Handler
in the Axis2 module. I'd suggest to remove it, but I may be missing
something...
However, MessageImpl is an SPI IMO, as various places in the runtime
including extensions should be able to new up a MessageImpl. Do people
think that it should be named differently? any proposal?
For implementation classes that implement SPI interfaces and need to
be newed up by extension code, a factory approach seems the right
solution. I'd be happy to make this change. Do we have any existing
similar factory pattern that I should follow?
3. DefaultAssemblyFactory is in the interface package
org.apache.tuscany.assembly, but it extends from AssemblyFactoryImpl
which is in the implementation package
org.apache.tuscany.assembly.impl.
This means that "impl" code is showing up indirectly as part of an
SPI interface. The same arrangement is used for DefaultPolicyFactory,
DefaultWSDLFactory, and DefaultWebServiceBindingFactory.
I don't find this shocking and I don't have a better (and reasonably
simple) idea to improve this. Do you have a proposal?
- move the XyzDefaultFactory to the impl package?
- get rid of XyzDefaultFactory and have the people new up a
XyzFactoryImpl directly?
- any other suggestion?
On further consideration, I think this pattern is OK because there's
no direct reference from extension code to any impl class. Other
arrangements are possible, such as:
package org.apache.tuscany.assembly;
import org.apache.tuscany.assembly.impl.AssemblyFactoryImpl;
public class DefaultAssemblyFactory {
private static AssemblyFactory defaultFactory;
public static AssemblyFactory getDefaultFactory() {
if (defaultFactory == null)
defaultFactory = new AssemblyFactoryImpl();
return defaultFactory;
}
}
I think this is slightly cleaner, but it's not a big deal one way
or the other.
4. There's a PrintUtil class in the org.apache.tuscany.assembly.util
package. Why isn't this in the impl subpackage?
I didn't want to clutter the assembly.impl package with a util class
that has nothing to do with the assembly model implementation.
OK, that makes sense.
5. The org.apache.tuscany.assembly.xml package seems to contain
implementation classes that aren't in an "impl" subpackage.
I'm not sure that we want all non spi packages to be *.impl. In general
I think we should avoid extremes, like:
- have all spis in *.spi.* packages
- or have all non spi in *.impl packages
Looking at other Apache projects, many seem to follow a more reasonable
and balanced approach, which I hope we can do as well.
I replied to this on a separate thread.
6. The bindings don't seem to follow the "impl" subpackage convention,
except for binding-ws.
7. None of the core subpackages follow the "impl" subpackage
convention. They are called component, event, invocation, runtime,
store, util, and work.
We need to refactor these packages, these modules do not offer SPIs so
it's not necessarily very bad. I'd suggest the following:
- open JIRAs for this, to avoid losing these issues in the flow of emails
- evaluate their priority and stage this work, I'd suggest to do this
kind of refactoring after we have stabilized the function
- find volunteers to do this work :)
The core does offer SPIs. These are in a separate core-spi module in the
svn repository, but when we build a release distribution they will (I presume)
be in the tuscany-all jar together with the core implementation code, and
not very clearly distinguished from it. For now I'll open a JIRA to track
the issue. I am willing to do this refactoring at a suitable time if we
can agree on a convention that everyone is OK with.
8. I'm not sure whether the core-databinding module follows the
convention. There is no "impl" subpackage, but there seems to be
some implementation code.
9. core-spring nearly follows the convention, with everything in "impl"
subpackages except for one class ComponentContext in the "runtime"
subpackage.
ComponentContext is a temporary class and should go away when SCADomain
becomes available for use in that environment (which will require a few
changes in it).
OK, thanks.
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
