[jboss-dev-forums] [Embedded JBoss Development] - Peer Review of Embedded, ShrinkWrap and Bootstrap

[ALRubinger]

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#4261505

Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=4261505