[xwiki-devs] Fwd: [Wiki Importer] Design questions
(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
Hi Vincent, On Sun, Jul 26, 2009 at 4:15 PM, Vincent Massol <[email protected]> wrote:
(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(...)
Wiki Interface is useful to access the Wiki Instance (MediaWiki , Confluence ) at runtime. The WikiImporter Interface first method processInputDump() process the Pages, Attachments and metadata and returns an object of Export Wiki Type( implements Wiki<T> Interface) which can be used to customize the import process. For Eg : Change in the Sitename, Preserve metadata and revision history. All such properties can be set at run time during import process.( From GUI - Wizard Interface) After customisation, second method createWikiPages() can be called on the same export type wiki object. which imports the pages to XWiki.
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.
Yes, that is an issue. During import process the dump is uploaded as Attachment. And the attachment content can be accessed as byte[] array only in the existing XWiki API. So the byte array is converted to ByteArrayStream and imported in the exisiting implementation.
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).
I liked the idea. Will make necessary changes to implement WikiImporterFactory.
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.
Does dependency means a Requirement ( Injecting using @Requirement ) ?
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 was not aware of existing api. I will change it.
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 [email protected] http://lists.xwiki.org/mailman/listinfo/devs
I have created a jira issue - Improving existing Wiki Importer at http://jira.xwiki.org/jira/browse/XSANDBOX-55 I will be happy to hear more comments from your side. Thanks and Best Regards, Arun Reddy
On Sun, Jul 26, 2009 at 20:18, Arun Reddy<[email protected]> wrote:
Hi Vincent,
On Sun, Jul 26, 2009 at 4:15 PM, Vincent Massol <[email protected]> wrote:
(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(...)
Wiki Interface is useful to access the Wiki Instance (MediaWiki , Confluence ) at runtime. The WikiImporter Interface first method processInputDump() process the Pages, Attachments and metadata and returns an object of Export Wiki Type( implements Wiki<T> Interface) which can be used to customize the import process.
For Eg : Change in the Sitename, Preserve metadata and revision history.
All such properties can be set at run time during import process.( From GUI - Wizard Interface)
After customisation, second method createWikiPages() can be called on the same export type wiki object. which imports the pages to XWiki.
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.
Yes, that is an issue. During import process the dump is uploaded as Attachment. And the attachment content can be accessed as byte[] array only in the existing XWiki API. So the byte array is converted to ByteArrayStream and imported in the exisiting implementation.
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).
I liked the idea. Will make necessary changes to implement WikiImporterFactory.
Would be even better than each importer be a component with syntax as role hint exactly like for parsers.
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.
Does dependency means a Requirement ( Injecting using @Requirement ) ?
Yes. @Requirement private ComponenetManager componenetManager;
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 was not aware of existing api. I will change it.
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 [email protected] http://lists.xwiki.org/mailman/listinfo/devs
I have created a jira issue - Improving existing Wiki Importer at http://jira.xwiki.org/jira/browse/XSANDBOX-55
I will be happy to hear more comments from your side.
Thanks and Best Regards, Arun Reddy _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne
Hi Arun, On Sun, Jul 26, 2009 at 8:18 PM, Arun Reddy<[email protected]> wrote:
Hi Vincent,
On Sun, Jul 26, 2009 at 4:15 PM, Vincent Massol <[email protected]> wrote:
(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(...)
Wiki Interface is useful to access the Wiki Instance (MediaWiki , Confluence ) at runtime.
I don't understand why it's useful since it doesn't have any interesting method. You can just access whatever specific instance you need for a given wiki. The WikiImporter Interface first method processInputDump()
process the Pages, Attachments and metadata and returns an object of Export Wiki Type( implements Wiki<T> Interface) which can be used to customize the import process.
For Eg : Change in the Sitename, Preserve metadata and revision history.
I don't understand why you need to expose this in the interface. A single import() seems more than enough to me. What wouldn't work with that method?
All such properties can be set at run time during import process.( From GUI - Wizard Interface)
You should have a WikiImporterConfiguration component for storing config properties.
After customisation, second method createWikiPages() can be called on the same export type wiki object. which imports the pages to XWiki.
Again why do you need to expose it in the public API?
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.
Yes, that is an issue. During import process the dump is uploaded as Attachment.
We shouldn't do this. It won't work a of now. We need to have the dump on the server's file system and read it from there. [snip]] Thanks -Vincent
On 2009-07-26, Vincent Massol <[email protected]> wrote:
(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 _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Sent from my mobile device
Hi Sergiu, Whats the process to be followed after making few improvements ( Refactoring ). Can I commit the code directly into Sandbox/gsoc/importexport ? -- Best Regards, Arun Reddy
On Mon, Jul 27, 2009 at 04:09, Arun Reddy<[email protected]> wrote:
Hi Sergiu,
Whats the process to be followed after making few improvements ( Refactoring ).
Can I commit the code directly into Sandbox/gsoc/importexport ?
Yes sure, since it's in sandbox it's your project so you should commit any improvement/fix directly (don't forget to link any commit to a jira issue in the commit comment) ;)
-- Best Regards, Arun Reddy _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne
participants (4)
-
Arun Reddy -
Bindul Bhowmik -
Thomas Mortagne -
Vincent Massol