11 Dec
2009
11 Dec
'09
2:54 p.m.
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.
Thanks
-Vincent