[GitHub] [maven-doxia] abelsromero commented on a change in pull request #35: [DOXIA-614] support source reference in doxia parser

[GitBox <gi...@apache.org>]

abelsromero commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r501162574



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/Xhtml5BaseParser.java
##########
@@ -81,14 +81,14 @@
 
     /** {@inheritDoc} */
     @Override
-    public void parse( Reader source, Sink sink )
+    public void parse( Reader source, Sink sink, String reference )

Review comment:
       Not really.
   
   To see the changes probably, the image in the PR description summarizes the changes (@ means overrride method), at least it helped me to track the changes.
   The problems is the `parse(String,String,String)` method in `AbstractParser` was calling `parse(String, String)` without the third argument. So the "reference" was totally lost and no one could use it.
   Even with fixing the call in `AbstractParser`, the problem is that all implementations add logic in their respective `parse` and relly on the others using `super.parse(String,String)` calls. So only that change would mean that someone overriding `Xhtml5Parser::parse(String,String,String)` would find a total different behauviour than extending the 2 args method because the will be extending the `AbstractParser` really and missing a lot of logic.
   
   The only solution I saw was to replace 2 args method by the 3 args in all implementations and call the 3 args in the `AbstractParser` with null. That makes `reference` available and should not brake current doxia components.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org