You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@maven.apache.org by GitBox <gi...@apache.org> on 2020/06/28 17:44:06 UTC

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

abelsromero opened a new pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35


   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.
   ![doxia-modules-classes](https://user-images.githubusercontent.com/5781153/85954350-2159c700-b977-11ea-939c-3a39b554b114.png).
   
   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.
   
   
   
   


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r446678771



##########
File path: doxia-core/src/test/java/org/apache/maven/doxia/DefaultDoxiaTest.java
##########
@@ -0,0 +1,54 @@
+package org.apache.maven.doxia;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.maven.doxia.parser.manager.ParserNotFoundException;
+import org.codehaus.plexus.PlexusTestCase;
+import org.junit.Test;
+
+public class DefaultDoxiaTest extends PlexusTestCase
+{
+
+    @Test
+    public void testCreatesDefaultDoxia()
+    {
+        final DefaultDoxia defaultDoxia = new DefaultDoxia();
+
+        assertNotNull( defaultDoxia );
+    }
+
+    @Test
+    public void testFailsWhenParserIdDoesNotExist() throws Exception
+    {
+        final String parserId = "a-parser";
+        final Doxia doxia = lookup( Doxia.class );
+
+        try
+        {
+            doxia.getParser( parserId );
+        }
+        catch ( Exception e )

Review comment:
       just catch ParserNotFoundException rather than asserting the type

##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java
##########
@@ -52,6 +52,22 @@
     void parse( Reader source, String parserId, Sink sink )
         throws ParserNotFoundException, ParseException;
 
+    /**
+     * Parses the given source model using a parser with given id,
+     * and emits Doxia events into the given sink.
+     *
+     * @param source not null reader that provides the source document.
+     * You could use <code>newReader</code> methods from {@link org.codehaus.plexus.util.ReaderFactory}.

Review comment:
       I'd delete this note. I'd prefer not to encourage extra plexus dependencies

##########
File path: doxia-core/src/test/java/org/apache/maven/doxia/DefaultDoxiaTest.java
##########
@@ -0,0 +1,54 @@
+package org.apache.maven.doxia;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.maven.doxia.parser.manager.ParserNotFoundException;
+import org.codehaus.plexus.PlexusTestCase;
+import org.junit.Test;
+
+public class DefaultDoxiaTest extends PlexusTestCase
+{
+
+    @Test
+    public void testCreatesDefaultDoxia()
+    {
+        final DefaultDoxia defaultDoxia = new DefaultDoxia();
+
+        assertNotNull( defaultDoxia );
+    }
+
+    @Test
+    public void testFailsWhenParserIdDoesNotExist() throws Exception
+    {
+        final String parserId = "a-parser";
+        final Doxia doxia = lookup( Doxia.class );
+
+        try
+        {
+            doxia.getParser( parserId );

Review comment:
       test should fail here if exception is not thrown

##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java
##########
@@ -52,6 +52,22 @@
     void parse( Reader source, String parserId, Sink sink )
         throws ParserNotFoundException, ParseException;
 
+    /**
+     * Parses the given source model using a parser with given id,
+     * and emits Doxia events into the given sink.
+     *
+     * @param source not null reader that provides the source document.
+     * You could use <code>newReader</code> methods from {@link org.codehaus.plexus.util.ReaderFactory}.
+     * @param parserId Identifier for the parser to use.
+     * @param sink A sink that consumes the Doxia events.

Review comment:
       ditto

##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java
##########
@@ -52,6 +52,22 @@
     void parse( Reader source, String parserId, Sink sink )
         throws ParserNotFoundException, ParseException;
 
+    /**
+     * Parses the given source model using a parser with given id,
+     * and emits Doxia events into the given sink.
+     *
+     * @param source not null reader that provides the source document.
+     * You could use <code>newReader</code> methods from {@link org.codehaus.plexus.util.ReaderFactory}.
+     * @param parserId Identifier for the parser to use.

Review comment:
       nit: no caps and no period, per Oracle guidelines




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r447151457



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java
##########
@@ -41,24 +41,35 @@
      * Parses the given source model using a parser with given id,
      * and emits Doxia events into the given sink.
      *
-     * @param source not null reader that provides the source document.
-     * You could use <code>newReader</code> methods from {@link org.codehaus.plexus.util.ReaderFactory}.
-     * @param parserId Identifier for the parser to use.
-     * @param sink A sink that consumes the Doxia events.
-     * @throws org.apache.maven.doxia.parser.manager.ParserNotFoundException
-     * if no parser could be found for the given id.
-     * @throws org.apache.maven.doxia.parser.ParseException if the model could not be parsed.
+     * @param source not null reader that provides the source document
+     * @param parserId identifier for the parser to use
+     * @param sink a sink that consumes the Doxia events
+     * @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 )
         throws ParserNotFoundException, ParseException;
 
+    /**
+     * Parses the given source model using a parser with given id,
+     * and emits Doxia events into the given sink.
+     *
+     * @param source not null reader that provides the source document
+     * @param parserId identifier for the parser to use
+     * @param sink a sink that consumes the Doxia events
+     * @param reference string containing the reference to the source (e.g. filename)
+     * @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 )
+        throws ParserNotFoundException, ParseException;
+
     /**
      * Return a parser for the given <code>parserId</code>.
      *
-     * @param parserId Identifier for the parser to use.
-     * @return the parser defining by parserId.
-     * @throws org.apache.maven.doxia.parser.manager.ParserNotFoundException
-     * if no parser could be found for the given id.
+     * @param parserId identifier for the parser to use
+     * @return the parser defining by parserId

Review comment:
       defining --> defined 
   
   or better yet 
   
   identified

##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java
##########
@@ -41,24 +41,35 @@
      * Parses the given source model using a parser with given id,
      * and emits Doxia events into the given sink.
      *
-     * @param source not null reader that provides the source document.
-     * You could use <code>newReader</code> methods from {@link org.codehaus.plexus.util.ReaderFactory}.
-     * @param parserId Identifier for the parser to use.
-     * @param sink A sink that consumes the Doxia events.
-     * @throws org.apache.maven.doxia.parser.manager.ParserNotFoundException
-     * if no parser could be found for the given id.
-     * @throws org.apache.maven.doxia.parser.ParseException if the model could not be parsed.
+     * @param source not null reader that provides the source document
+     * @param parserId identifier for the parser to use
+     * @param sink a sink that consumes the Doxia events
+     * @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 )
         throws ParserNotFoundException, ParseException;
 
+    /**
+     * Parses the given source model using a parser with given id,
+     * and emits Doxia events into the given sink.
+     *
+     * @param source not null reader that provides the source document
+     * @param parserId identifier for the parser to use
+     * @param sink a sink that consumes the Doxia events
+     * @param reference string containing the reference to the source (e.g. filename)
+     * @throws ParserNotFoundException If no parser could be found for the given id

Review comment:
       If --> if

##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractParser.java
##########
@@ -179,15 +178,32 @@ protected File getBasedir()
     public void parse( String string, Sink sink )
         throws ParseException
     {
-        parse( new StringReader( string ), sink );
+        this.parse( string, sink, null );
     }
-    
+
+    /**
+     * {@inheritDoc}
+     *
+     * Convenience method to parse an arbitrary string and emit events into the given sink.
+     *
+     * @param string A string that provides the source input.
+     * @param sink A sink that consumes the Doxia events.

Review comment:
       A --> a
   no period
   
   ditto below

##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractXmlParser.java
##########
@@ -100,7 +98,7 @@
     private boolean validate = false;
 
     /** {@inheritDoc} */
-    public void parse( Reader source, Sink sink )
+    public void parse( Reader source, Sink sink, String reference )

Review comment:
       Does this change the public API? I'm not sure these changes are compatible. 

##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java
##########
@@ -41,24 +41,35 @@
      * Parses the given source model using a parser with given id,
      * and emits Doxia events into the given sink.
      *
-     * @param source not null reader that provides the source document.
-     * You could use <code>newReader</code> methods from {@link org.codehaus.plexus.util.ReaderFactory}.
-     * @param parserId Identifier for the parser to use.
-     * @param sink A sink that consumes the Doxia events.
-     * @throws org.apache.maven.doxia.parser.manager.ParserNotFoundException
-     * if no parser could be found for the given id.
-     * @throws org.apache.maven.doxia.parser.ParseException if the model could not be parsed.
+     * @param source not null reader that provides the source document
+     * @param parserId identifier for the parser to use
+     * @param sink a sink that consumes the Doxia events
+     * @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 )
         throws ParserNotFoundException, ParseException;
 
+    /**
+     * Parses the given source model using a parser with given id,
+     * and emits Doxia events into the given sink.
+     *
+     * @param source not null reader that provides the source document
+     * @param parserId identifier for the parser to use
+     * @param sink a sink that consumes the Doxia events
+     * @param reference string containing the reference to the source (e.g. filename)
+     * @throws ParserNotFoundException If no parser could be found for the given id
+     * @throws ParseException If the model could not be parsed

Review comment:
       If --> if




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
abelsromero commented on pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#issuecomment-654648699


   Applied the suggestions. Anything else that could be improved?


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r447236450



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java
##########
@@ -52,6 +52,22 @@
     void parse( Reader source, String parserId, Sink sink )
         throws ParserNotFoundException, ParseException;
 
+    /**
+     * Parses the given source model using a parser with given id,
+     * and emits Doxia events into the given sink.
+     *
+     * @param source not null reader that provides the source document.
+     * You could use <code>newReader</code> methods from {@link org.codehaus.plexus.util.ReaderFactory}.
+     * @param parserId Identifier for the parser to use.

Review comment:
       I assume that's an oversight since it contradicts the language of the doc and only appears in an example




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
abelsalgadoromeroWD commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r447950882



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractParser.java
##########
@@ -37,6 +29,13 @@
 import org.apache.maven.doxia.sink.Sink;
 import org.codehaus.plexus.component.annotations.Requirement;
 
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.Reader;
+import java.io.StringReader;
+import java.util.Properties;
+

Review comment:
       It's by Intellij, I'll restore it in a commit. I have noted many other things that even with the style configured as described in https://maven.apache.org/developers/conventions/code.html#intellij-idea. For example exception below the method definition are indented at 8 spaces while the original code is at 4.
   I try to keep some things that I see, but some may slipt, sorry.




----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r501060691



##########
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:
       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?




----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
abelsromero commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r447224575



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractParser.java
##########
@@ -179,15 +178,32 @@ protected File getBasedir()
     public void parse( String string, Sink sink )
         throws ParseException
     {
-        parse( new StringReader( string ), sink );
+        this.parse( string, sink, null );
     }
-    
+
+    /**
+     * {@inheritDoc}
+     *
+     * Convenience method to parse an arbitrary string and emit events into the given sink.
+     *
+     * @param string A string that provides the source input.
+     * @param sink A sink that consumes the Doxia events.

Review comment:
       If all the JavaDocs need to be reviewed, these maybe should be addressed in a specific effort with a specific JIRA issue. I just added the information related to the new parameter `reference` following the patterns already pressent in the project.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
hboutemy commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r447273709



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractParser.java
##########
@@ -37,6 +29,13 @@
 import org.apache.maven.doxia.sink.Sink;
 import org.codehaus.plexus.component.annotations.Requirement;
 
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.Reader;
+import java.io.StringReader;
+import java.util.Properties;
+

Review comment:
       is it possible to avoid such reformatting of imports, please?




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
abelsromero commented on pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#issuecomment-651269972


   Applied the suggestions tried to keep commit history clean.
   - Squashed test change in Test commit
   - Added new commit for Doxia's JavaDoc review


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
abelsromero commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r447951195



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractParser.java
##########
@@ -37,6 +29,13 @@
 import org.apache.maven.doxia.sink.Sink;
 import org.codehaus.plexus.component.annotations.Requirement;
 
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.Reader;
+import java.io.StringReader;
+import java.util.Properties;
+

Review comment:
       It's by Intellij, I'll restore it in a commit. I have noted many other things that even with the style configured as described in https://maven.apache.org/developers/conventions/code.html#intellij-idea. For example exception below the method definition are indented at 8 spaces while the original code is at 4.
   I try to keep some things that I see, but some may slipt, sorry.




----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
abelsromero commented on pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#issuecomment-755431992


   Done :+1: 


----------------------------------------------------------------
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

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

Posted by 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

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

Posted by GitBox <gi...@apache.org>.
abelsromero commented on pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#issuecomment-652029715


   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.


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
elharo commented on pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#issuecomment-754590700


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


----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
abelsromero commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r446685351



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java
##########
@@ -52,6 +52,22 @@
     void parse( Reader source, String parserId, Sink sink )
         throws ParserNotFoundException, ParseException;
 
+    /**
+     * Parses the given source model using a parser with given id,
+     * and emits Doxia events into the given sink.
+     *
+     * @param source not null reader that provides the source document.
+     * You could use <code>newReader</code> methods from {@link org.codehaus.plexus.util.ReaderFactory}.
+     * @param parserId Identifier for the parser to use.

Review comment:
       The exceptions in the oracle reference (https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#@exception) do capitalize, should I correct exceptions also accordingly?




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
slachiewicz commented on pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#issuecomment-850991975


   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/ 


-- 
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

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

Posted by GitBox <gi...@apache.org>.
hboutemy commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r447273790



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractXmlParser.java
##########
@@ -49,18 +32,33 @@
 import org.apache.maven.doxia.sink.impl.SinkEventAttributeSet;
 import org.apache.maven.doxia.util.HtmlTools;
 import org.apache.maven.doxia.util.XmlValidator;
-
 import org.codehaus.plexus.util.FileUtils;
 import org.codehaus.plexus.util.IOUtil;
 import org.codehaus.plexus.util.StringUtils;
 import org.codehaus.plexus.util.xml.pull.MXParser;
 import org.codehaus.plexus.util.xml.pull.XmlPullParser;
 import org.codehaus.plexus.util.xml.pull.XmlPullParserException;
-
 import org.xml.sax.EntityResolver;
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
 
+import java.io.BufferedReader;

Review comment:
       is it possible to avoid such reformatting of imports, please?




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
slachiewicz commented on pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#issuecomment-677469957


   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 ?


----------------------------------------------------------------
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

[GitHub] [maven-doxia] asfgit closed pull request #35: [DOXIA-614] support source reference in doxia parser

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35


   


-- 
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r552855714



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractParser.java
##########
@@ -101,7 +101,7 @@
     /**
      * {@inheritDoc}
      *
-     * @return a int.
+     * @return a int

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

##########
File path: 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

Review comment:
       ditto

##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java
##########
@@ -41,24 +41,35 @@
      * Parses the given source model using a parser with given id,
      * and emits Doxia events into the given sink.
      *
-     * @param source not null reader that provides the source document.
-     * You could use <code>newReader</code> methods from {@link org.codehaus.plexus.util.ReaderFactory}.
-     * @param parserId Identifier for the parser to use.
-     * @param sink A sink that consumes the Doxia events.
-     * @throws org.apache.maven.doxia.parser.manager.ParserNotFoundException
-     * if no parser could be found for the given id.
-     * @throws org.apache.maven.doxia.parser.ParseException if the model could not be parsed.
+     * @param source not null reader that provides the source document
+     * @param parserId identifier for the parser to use
+     * @param sink a sink that consumes the Doxia events
+     * @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 )
         throws ParserNotFoundException, ParseException;
 
+    /**
+     * Parses the given source model using a parser with given id,
+     * and emits Doxia events into the given sink.
+     *
+     * @param source not null reader that provides the source document
+     * @param parserId identifier for the parser to use
+     * @param sink a sink that consumes the Doxia events
+     * @param reference string containing the reference to the source (e.g. filename)
+     * @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 )

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




----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
abelsromero commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r451014972



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractXmlParser.java
##########
@@ -100,7 +98,7 @@
     private boolean validate = false;
 
     /** {@inheritDoc} */
-    public void parse( Reader source, Sink sink )
+    public void parse( Reader source, Sink sink, String reference )

Review comment:
       Effectively, it is not. As explained in the JIRA issue the PR aims to be that the default method instead of the other one with 2 arguments. The way it is implemented right now, even when `Parser` interface has two method, the one with thre is not usable by how it's overriden in `AbstractParser` and never invoked in `DefaultDoxia`.
   The final goal is to deprecate the one with two arguments so that any plugin can obtain the metadata of the sourcefile bien processed.
   
   I am available to discuss online if anyone needs to, because I am bit confused by how this review is proceding. I think the JIRA issue, explanation here and the additional diagram provide a clear description of the goals and changes done which are the complex topics. However, I am seeing a focus on minor things. Furthermore, I raised 2 important points in the JIRA issue which had had no answer and which imo are worth discussing, one related to problems with the build and the other with testing.




----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
abelsromero commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r447219577



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractXmlParser.java
##########
@@ -100,7 +98,7 @@
     private boolean validate = false;
 
     /** {@inheritDoc} */
-    public void parse( Reader source, Sink sink )
+    public void parse( Reader source, Sink sink, String reference )

Review comment:
       Please, refer to the JIRA for the conversation https://issues.apache.org/jira/browse/DOXIA-614




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r450756476



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractXmlParser.java
##########
@@ -100,7 +98,7 @@
     private boolean validate = false;
 
     /** {@inheritDoc} */
-    public void parse( Reader source, Sink sink )
+    public void parse( Reader source, Sink sink, String reference )

Review comment:
       The reference argument doesn't seem to be used here. 




----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
abelsromero commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r447224915



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/Doxia.java
##########
@@ -41,24 +41,35 @@
      * Parses the given source model using a parser with given id,
      * and emits Doxia events into the given sink.
      *
-     * @param source not null reader that provides the source document.
-     * You could use <code>newReader</code> methods from {@link org.codehaus.plexus.util.ReaderFactory}.
-     * @param parserId Identifier for the parser to use.
-     * @param sink A sink that consumes the Doxia events.
-     * @throws org.apache.maven.doxia.parser.manager.ParserNotFoundException
-     * if no parser could be found for the given id.
-     * @throws org.apache.maven.doxia.parser.ParseException if the model could not be parsed.
+     * @param source not null reader that provides the source document
+     * @param parserId identifier for the parser to use
+     * @param sink a sink that consumes the Doxia events
+     * @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 )
         throws ParserNotFoundException, ParseException;
 
+    /**
+     * Parses the given source model using a parser with given id,
+     * and emits Doxia events into the given sink.
+     *
+     * @param source not null reader that provides the source document
+     * @param parserId identifier for the parser to use
+     * @param sink a sink that consumes the Doxia events
+     * @param reference string containing the reference to the source (e.g. filename)
+     * @throws ParserNotFoundException If no parser could be found for the given id
+     * @throws ParseException If the model could not be parsed

Review comment:
       Please refer to my previous question https://github.com/apache/maven-doxia/pull/35#discussion_r446685351




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
abelsromero commented on a change in pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#discussion_r447219577



##########
File path: doxia-core/src/main/java/org/apache/maven/doxia/parser/AbstractXmlParser.java
##########
@@ -100,7 +98,7 @@
     private boolean validate = false;
 
     /** {@inheritDoc} */
-    public void parse( Reader source, Sink sink )
+    public void parse( Reader source, Sink sink, String reference )

Review comment:
       Please, refer to the JIRA tp follow on this topic: https://issues.apache.org/jira/browse/DOXIA-614. This is indeed no compatible but, necessary. Moreover it seems this change was already planed.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org

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

Posted by GitBox <gi...@apache.org>.
abelsromero commented on pull request #35:
URL: https://github.com/apache/maven-doxia/pull/35#issuecomment-736798699


   Anything I should review?


----------------------------------------------------------------
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