Hi Vincent, On Fri, Dec 11, 2009 at 7:24 PM, Vincent Massol <[email protected]> wrote:

Hi Asiri,

This is mostly for you but sending here for others to see and comment if they want. While working on adding support for wiki macro visibility I have found the following issues in the existing wiki macro code:

1) Usage of String for document names instead of DocumentName class. I've fixed this. 2) Printing logs in all methods instead of sending exceptions on errors. I've fixed this too. 3) Bad test strategy for testing DefaultWikiMacro. It's not needed to use RenderingTests with test files. I've fixed this. 4) Missing several tests in DefaultWikiMacro. I have not fixed this. 5) Bad strategy for providing user feedback on macro registration. I believe we shouldn't implement registration as an event listener and instead have a Wiki Macro management page with register/unregister buttons and when clicked the result is printed on the page. This should probably be done as a replacement of the WikiMacros page in the wiki macro bridge application. WDYT? I've not fixed this. 6) Programming rights required to access macro params. I have not fixed this. 7) There was a bug with parameter ordering I have fixed a few weeks ago but I'm mentionning it here for completeness. 8) Context restoration in DefaultWikiMacro. It's currently not in a finally block and thus it's possible it's not restored if there are exceptions raised. I have not fixed this. 9) Missing some comments for complex code portions. For ex in DefaultWikiMacro for the inline macro stuff. I have not fixed this. 10) Systematic setting of null values not required in lots of place. I have fixed all I saw. 11) DefaultWikiMacro only executes Macro Transformations but not other Transformations. I think this is a problem. I have not fixed this. 12) Several classes had full javadoc but the checkstyle config wasn't set in the POM, thus allowing checkstyle errors to creep in unnoticed. I have only partially fixed this. Please check all classes that can be added and fix the few that still have errors. 13) Missing tests for the Event Listener but because of 5) I haven't added them.

14) Missing at least one functional test to prove everything works.

Many thanks for the review. I agree with all the issues you have mentioned. Now, except for 5) I would like to address rest of the issues asap (one by one, alongside my other work). I think I discussed about 6) on the list about a month ago (http://www.mail-archive.com/[email protected]/msg10635.html) but got no replies. I will need to do a small recap and start a new discussion on this topic. Thanks. - Asiri

Thanks -Vincent

_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs