[jboss-dev-forums] [Embedded JBoss Development] - Peer Review of Embedded, ShrinkWrap and Bootstrap
Jesper was kind enough to have a look at our work and give his input.
"Jesper Pedesen" wrote :
| Overall:
| ========
|
| * Good idea to add overview.html / package.html
| * Good idea to run checkstyle
| * Good idea to run findbugs (with reportLevel="low")
|
| Bootstrap:
| ==========
| * api-embedded
| * impl-embedded
|
| - Needs to be move to Embedded - as they are a specific use-case of Bootstrap
| - Refactor package names
|
| * I'm not too crazy about BootstrapMetadata being in the spi/ package
| * Also there are constants in spi/ which may not have a relevance for the actual
environment
|
| * I guess it is ok to keep the MC and AS specific modules here - as many projects will
use them
|
| * Missing excludes in pom.xml for MC and AS
|
| org.jboss.bootstrap.api.server.Server
| -------------------------------------
| void registerEventHandler(LifecycleState state, LifecycleEventHandler handler) throws
IllegalArgumentException;
|
| arguments should be reversed to follow other registerEventHandler methods
|
| org.jboss.bootstrap.api.lifecycle.LifecycleState
| ------------------------------------------------
|
| Are we sure that there won't be any additional server states in the future ?
| Or that an implementation will define additional states ?
|
| In those cases an enum won't work - as it can't be extended.
|
| Embedded:
| =========
|
| * Missing excludes in pom.xml
|
| org.jboss.bootstrap.api.embedded.server.JBossASEmbeddedServerFactory
| --------------------------------------------------------------------
|
| "public static final ..." -> "private static final ..."
|
| ... otherwise a
|
| createServer(final ClassLoader cl, final String className)
|
| method is needed...
|
|
| ShrinkWrap:
| ===========
|
| - Check ContextClassLoader assumptions
|
| org.jboss.shrinkwrap.api.Path
| -----------------------------
|
| * Would a org.jboss.cache.Fqn style be better ?
|
| org.jboss.shrinkwrap.api.Archive
| --------------------------------
|
| T add(Path target, String name, Asset asset) throws IllegalArgumentException;
|
| switch assert and name
|
| * Remove all "String target" -- Path should be used
|
| T merge(Path path, Archive<?> source) throws IllegalArgumentException;
|
| switch path and source
|
|
| org.jboss.shrinkwrap.api.spec.FactoryUtil
| -----------------------------------------
|
| * createInstance method with ClassLoader
|
| org.jboss.shrinkwrap.api.container.ResourceContainer
| ----------------------------------------------------
|
| * Alignment of method arguments
|
| org.jboss.shrinkwrap.api.container.EnterpriseContainer
| ------------------------------------------------------
|
| * Alignment of method arguments
|
| org.jboss.shrinkwrap.api.container.WebContainer
| -----------------------------------------------
|
| * Alignment of method arguments
|
| org.jboss.shrinkwrap.api.container.ManifestContainer
| ----------------------------------------------------
|
| * Alignment of method arguments
|
| org.jboss.shrinkwrap.api.container.LibraryContainer
| ---------------------------------------------------
|
| * addLibraries()
|
| org.jboss.shrinkwrap.api.export.FactoryUtil
| -------------------------------------------
|
| * createInstance() with ClassLoader
View the original post :
http://www.jboss.org/index.html?module=bb&op=viewtopic&p=4261505#...
Reply to the post :
http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&a...