[DOXIASITETOOLS-324] Allow configuration of Doxia parser

[kwin]

Requires #121

[michael-o]

Am I stupid or why not just return the object?

[kwin]

because this is not supposed to return null ever (will clarify in the javadoc).

[michael-o]

I'll need a bit more time. I need to review this in tandem with the Maven Site Plugin PR.

[kwin]

@michael-o Not sure why the build fails now, seems related to #124 but not sure why master is not affected. Maybe we can merge #124 first.

Update: This wasn't related, rather the test cases need to be adjusted now that the default is generating anchors for indexable entries.

[michael-o]

My first (incomplete) analysis: I believe that this has at least two design flaws:

• The matching is overengineered. We always operate on parser modules consistently, thus I would expect that a parser config applies on a per-module basis. A map holding the moduleId as key and the config as value are enough.
• The parser config isn't flexible: Consider someone has its own parsing module or has replaced ours and their parsers have more config properties/options, a plain class with getters and setters in Maven Site Plugin won't be able to populate them. A map would also be appropriate here.

Let me know what you think...

[kwin]

We always operate on parser modules consistently, thus I would expect that a parser config applies on a per-module basis.

You are probably right, a config mapping per id seems sufficient here.

The parser config isn't flexible

Right, but changing it would either require to add something like Parser.configure(Map<String,String> config) to Doxia which kind of conflicts with the existing setters or some sophisticated reflection to call the right setter. The tricky part is not to call the right setter method but to convert the parameter to the right type (leveraging the Plexus conversion logic). Which way of implementing this did you have in mind?

[michael-o]

We always operate on parser modules consistently, thus I would expect that a parser config applies on a per-module basis.

You are probably right, a config mapping per id seems sufficient here.

Yes

The parser config isn't flexible

Right, but changing it would either require to add something like Parser.configure(Map<String,String> config) to Doxia which kind of conflicts with the existing setters or some sophisticated reflection to call the right setter. The tricky part is not to call the right setter method but to convert the parameter to the right type (leveraging the Plexus conversion logic). Which way of implementing this did you have in mind?

About this, I need to think, but you are heading for that direction. I need more time. Can you make the first one happen as long as I think about the first one?

[michael-o]

@kwin Can you please double-check tests, they fail for me:

[INFO] Running org.apache.maven.doxia.siterenderer.DefaultSiteRendererTest
[main] WARN org.apache.maven.doxia.module.xdoc.XdocParser - Unrecognized xdoc tag <base> at [32:45]
[ERROR] Tests run: 7, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 4.820 s <<< FAILURE! -- in org.apache.maven.doxia.siterenderer.DefaultSiteRendererTest
[ERROR] org.apache.maven.doxia.siterenderer.DefaultSiteRendererTest.testRender -- Time elapsed: 2.650 s <<< ERROR!
java.lang.ClassCastException: org.htmlunit.html.HtmlAnchor cannot be cast to org.htmlunit.html.HtmlHeading1
        at org.apache.maven.doxia.siterenderer.NestedItemsVerifier.verify(NestedItemsVerifier.java:65)
        at org.apache.maven.doxia.siterenderer.DefaultSiteRendererTest.verifyNestedItemsPage(DefaultSiteRendererTest.java:353)
        at org.apache.maven.doxia.siterenderer.DefaultSiteRendererTest.testRender(DefaultSiteRendererTest.java:224)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at java.util.ArrayList.forEach(ArrayList.java:1259)
        at java.util.ArrayList.forEach(ArrayList.java:1259)

[INFO] Running org.apache.maven.doxia.siterenderer.RenderingContextTest
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.085 s -- in org.apache.maven.doxia.siterenderer.RenderingContextTest
[INFO]
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR]   DefaultSiteRendererTest.testRender:224->verifyNestedItemsPage:353 � ClassCast org.htmlunit.html.HtmlAnchor cannot be cast to org.htmlunit.html.HtmlHeading1

[kwin]

I know about the failing tests: #126 (comment)

[michael-o]

I know about the failing tests: #126 (comment)

Though, #124 has been merged. Should I ignore this for now or do you want to address it first?

[michael-o]

@kwin Can you take care of the failing tests? I want to complete the review this month.

[kwin]

There is some issues with the tests I have not fully understood yet (duplicate anchors). Therefore putting this PR back to draft modus. @michael-o Have you thought about using reflection or another way of configuring the parser?

[michael-o]

There is some issues with the tests I have not fully understood yet (duplicate anchors). Therefore putting this PR back to draft modus. @michael-o Have you thought about using reflection or another way of configuring the parser?

I have a few ideas, but am currently overloaded to dive in deeply. Hoping to complete this month. I think it might be worthwhile to ask @abelsromero as well for Asciidoctor extension.

[kwin]

Superseded by #140