(resending since I got a failure from xwiki's mail content filter ) ---------- Forwarded message ---------- From: Vincent Massol <vincent(a)massol.net> Date: Sun, Jul 26, 2009 at 11:33 AM Subject: [Wiki Importer] Design questions To: XWiki Developers <devs(a)xwiki.org> 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 _______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs