[DOXIA-614] support source reference in doxia parser #35

abelsromero · 2020-06-28T17:43:51Z

There's still a thing to fix with test begin discussed in https://issues.apache.org/jira/browse/DOXIA-614.
Other than that, the changes are all done and the tests pass. I tried to minimize changes so no tests have been modified and all works as-is.
To facilitate review, here is a diagram with changes. Green for added elements, red for removed.
.

Some observations open for discussion:

Used null insteall of empty string to represent there's no reference at all. I found the empty string to be ambiguous. I noted that AptParser, ConfluenceParser and TwikiParser use empty string, but I consider this implementation specific.

doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java

doxia-core/src/test/java/org/apache/maven/doxia/DefaultDoxiaTest.java

abelsromero · 2020-06-29T17:53:22Z

Applied the suggestions tried to keep commit history clean.

Squashed test change in Test commit
Added new commit for Doxia's JavaDoc review

doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java

doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractParser.java

doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractXmlParser.java

doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractParser.java

doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractXmlParser.java

abelsromero · 2020-06-30T20:37:46Z

All improvements applied, thanks for your time.
Note there are a couple of points for discussion I added in the JIRA issue. I am new, so let me know where do you think is best to discuss, directly here or there.

abelsromero · 2020-07-07T07:14:58Z

Applied the suggestions. Anything else that could be improved?

slachiewicz · 2020-08-20T08:55:04Z

ok, now we have green build (clirr update was missing)
CI: https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-doxia/job/DOXIA-614%252Fsupport-source-reference-in-doxia-parser/
branch: https://github.com/apache/maven-doxia/commits/DOXIA-614/support-source-reference-in-doxia-parser

I'm ok with changes, but it would b good to bump to 1.10 with proposed change wdyt @hboutemy ?

elharo · 2020-10-07T14:30:50Z

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 )


It's hard to follow from the diff, but are any of the apparent method signature changes real? Or is it just Github diff being funky?

If these are real changes is it possible to add overloads with the old variants that pass null for reference?

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.

abelsromero · 2020-12-01T20:26:09Z

Anything I should review?

elharo · 2021-01-05T11:54:17Z

If you resolve conflicts and ping me, I can take another look.

… for modules

…arser

abelsromero · 2021-01-06T17:07:10Z

Done 👍

elharo · 2021-01-06T17:49:23Z

doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java

+     * @throws ParserNotFoundException if no parser could be found for the given id
+     * @throws ParseException if the model could not be parsed
+     */
+    void parse( Reader source, String parserId, Sink sink, String reference )


probably doesn't matter a great deal, but adding methods to interfaces is not backwards compatible

elharo · 2021-01-06T17:49:49Z

doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractParser.java

@@ -101,7 +101,7 @@
    /**
     * {@inheritDoc}
     *
-     * @return a int.
+     * @return a int


I'd just delete this, if there's nothing more to say than the type

elharo · 2021-01-06T17:49:56Z

doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractParser.java

@@ -117,7 +117,7 @@ public void setEmitComments( boolean emitComments )
    /**
     * <p>isEmitComments.</p>
     *
-     * @return a boolean.
+     * @return a boolean


elharo

Jenkins failed: https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-doxia/job/DOXIA-614B/

slachiewicz · 2021-05-30T12:25:43Z

ok, fixed the issue and made a small adjustment. Build gene https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-doxia/job/DOXIA-614%252Fsupport-source-reference-in-doxia-parser/

Closes #35

elharo reviewed Jun 28, 2020

View reviewed changes

abelsromero force-pushed the DOXIA-614/support-source-reference-in-doxia-parser branch from 10928cb to 84bf84a Compare June 29, 2020 17:52

elharo reviewed Jun 29, 2020

View reviewed changes

hboutemy reviewed Jun 29, 2020

View reviewed changes

doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractParser.java Outdated Show resolved Hide resolved

hboutemy reviewed Jun 29, 2020

View reviewed changes

doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractXmlParser.java Outdated Show resolved Hide resolved

abelsromero force-pushed the DOXIA-614/support-source-reference-in-doxia-parser branch from 84bf84a to d99011c Compare June 30, 2020 20:34

elharo changed the title ~~DOXIA-614: support source reference in doxia parser~~ [DOXIA-614] support source reference in doxia parser Oct 7, 2020

elharo reviewed Oct 7, 2020

View reviewed changes

abelsromero mentioned this pull request Oct 11, 2020

Better error/warning log handling asciidoctor/asciidoctor-maven-plugin#323

Closed

abelsromero added 6 commits January 6, 2021 18:04

[DOXIA-614] Make Parser::parse(Reader,Sink,String) the default method…

a55438b

… for modules

[DOXIA-614] Support passing source reference to Doxia instance

e8f8c82

[DOXIA-614] Fix Doxia JavaDoc comments

bca3f5f

[DOXIA-614] Restore imports order for AbstractParser and AbstractXmlP…

b33226c

…arser

[DOXIA-614] Fix AbstractParser JavaDoc comments

50104e4

[DOXIA-614] Improve Doxia::getParser return JavaDoc

6c076ba

abelsromero force-pushed the DOXIA-614/support-source-reference-in-doxia-parser branch from 333a89e to 6c076ba Compare January 6, 2021 17:05

elharo approved these changes Jan 6, 2021

View reviewed changes

elharo requested changes May 30, 2021

View reviewed changes

asfgit closed this in 64ddf82 May 30, 2021

asfgit pushed a commit that referenced this pull request May 30, 2021

[DOXIA-614] Support passing source reference to Doxia instance

497a438

Closes #35

abelsromero mentioned this pull request Dec 21, 2021

[DOXIA-614] Pass input file name as reference to parser apache/maven-doxia-sitetools#19

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DOXIA-614] support source reference in doxia parser #35

[DOXIA-614] support source reference in doxia parser #35

abelsromero commented Jun 28, 2020

abelsromero commented Jun 29, 2020

abelsromero commented Jun 30, 2020

abelsromero commented Jul 7, 2020

slachiewicz commented Aug 20, 2020

elharo Oct 7, 2020

abelsromero Oct 7, 2020

abelsromero commented Dec 1, 2020

elharo commented Jan 5, 2021

abelsromero commented Jan 6, 2021

elharo Jan 6, 2021

elharo Jan 6, 2021

elharo Jan 6, 2021

elharo left a comment

slachiewicz commented May 30, 2021

[DOXIA-614] support source reference in doxia parser #35

[DOXIA-614] support source reference in doxia parser #35

Conversation

abelsromero commented Jun 28, 2020

abelsromero commented Jun 29, 2020

abelsromero commented Jun 30, 2020

abelsromero commented Jul 7, 2020

slachiewicz commented Aug 20, 2020

elharo Oct 7, 2020

Choose a reason for hiding this comment

abelsromero Oct 7, 2020

Choose a reason for hiding this comment

abelsromero commented Dec 1, 2020

elharo commented Jan 5, 2021

abelsromero commented Jan 6, 2021

elharo Jan 6, 2021

Choose a reason for hiding this comment

elharo Jan 6, 2021

Choose a reason for hiding this comment

elharo Jan 6, 2021

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

slachiewicz commented May 30, 2021