(resending since I got a failure from xwiki's mail content filter ) ---------- Forwarded message ---------- From: Vincent Massol <[email protected]> Date: Sun, Jul 26, 2009 at 11:33 AM Subject: [Wiki Importer] Design questions To: XWiki Developers <[email protected]> Hi Arun, I've just started reviewing your module. I'll send questions as they come. Here are some to start with: 1) Why do you need the Wiki interface? Since all implementations are wiki-specific and since it doesn't contain any useful method I don't see why it's needed. 2) Why do you need 2 methods in WikiImporter interface? I would have imagine only a single method: WikiImporter.import(...) 3) Why do you pass a byte array instead of a stream? If the import is large a byte array will result in an OOM error. 4) WikiImporterVelocityContextInitializer has invalid javadoc. In addition instead of exposing each importer you could expose only a single importer factory and pass an import syntax to it. This will reduce the coupling and not expose the implementations to the client side (very important). Something like: WikiImporterFactory.createImporter(ImportSyntax). 5) This should never be written (in DefaultImportWikiParser & DefaultImportWikiRenderer for ex): //Intialize Embeddable Component Manager EmbeddableComponentManager ecm = new EmbeddableComponentManager(); ecm.initialize(this.getClass().getClassLoader()); The component manager must alway be passed as a dependency. 6) You shouldn't use @Component("default") but instead @Component 7) You should never use new File(System.getProperty("java.io.tmpdir"). Instead you should use the existing API located in Container.getApplicationContext().getTemporaryDirectory(). I have some more comments but since they are linked to the above let's agree first on the points above before going further. Thanks -Vincent