You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/05/29 19:03:32 UTC
[GitHub] [maven-javadoc-plugin] XenoAmess opened a new pull request #48: fix javadoc; fix code smells; performance improvement; add travis-ci script.
XenoAmess opened a new pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement; add travis-ci script.
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r436295368
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractFixJavadocMojo.java
##########
@@ -808,20 +808,6 @@ private void parseClirrTextOutputFile( File clirrTextOutputFile )
switch ( code )
{
case 7011:
Review comment:
> fallthrough is dangerous; perhpas extrat a helper method instead
@elharo OK.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-676557809
@elharo
Thanks.
As the codes are clean now, I can continue to next step.
The next pr will be some fix about some bugs in javadoc:fix .
I will try to do it as fast as possible.
Will create pr when I finish.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443126587
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
##########
@@ -629,35 +650,34 @@ protected static String parseJavadocMemory( String memory )
throw new IllegalArgumentException( "The memory could not be null." );
}
- Pattern p = Pattern.compile( "^\\s*(\\d+)\\s*?\\s*$" );
+ Pattern p = PARSE_JAVADOC_MEMORY_PATTERN_0;
Matcher m = p.matcher( memory );
if ( m.matches() )
{
return m.group( 1 ) + "m";
}
-
- p = Pattern.compile( "^\\s*(\\d+)\\s*k(b)?\\s*$", Pattern.CASE_INSENSITIVE );
+ p = PARSE_JAVADOC_MEMORY_PATTERN_1;
m = p.matcher( memory );
if ( m.matches() )
{
return m.group( 1 ) + "k";
}
- p = Pattern.compile( "^\\s*(\\d+)\\s*m(b)?\\s*$", Pattern.CASE_INSENSITIVE );
+ p = PARSE_JAVADOC_MEMORY_PATTERN_2;
Review comment:
@elharo done.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement; add travis-ci script.
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r436295088
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
##########
@@ -629,35 +650,34 @@ protected static String parseJavadocMemory( String memory )
throw new IllegalArgumentException( "The memory could not be null." );
}
- Pattern p = Pattern.compile( "^\\s*(\\d+)\\s*?\\s*$" );
+ Pattern p = PARSE_JAVADOC_MEMORY_PATTERN_0;
Review comment:
@elharo basic idea is the original code compile the pattern every time it invoke, but actually we need compile it only one time.
after compile, it can be used thus do not need to compile it again when next try.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443136388
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
> There's something I'm not seeing here. Have you tried simply removing the catch block completely? Does that compile?
after removing that part:
```
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 12.622 s
[INFO] Finished at: 2020-06-20T22:46:09+08:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project maven-javadoc-plugin: Compilation failure
[ERROR] /D:/workspace/maven-javadoc-plugin/src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java:[6068,30] unreported exception java.io.IOException; must be caught or declared to be thrown
[ERROR] exception thrown from implicit call to close() on resource variable 'javadocClassLoader'
[ERROR]
[ERROR] -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
```
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443130157
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
@elharo no other source will throw IOException here.
`public URL getResource(String name) {`
IOException is not a RuntimeException. If It can be thrown it must imply it.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443133256
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
@elharo It seems only can throw when closing the `javadocClassLoader`. the original codes ignore it in `finally`. the new codes ignore it in `catch`. same thing IMO.
----------------------------------------------------------------
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-javadoc-plugin] elharo merged pull request #48: [MJAVADOC-653] fix javadoc; fix code smells
Posted by GitBox <gi...@apache.org>.
elharo merged pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess edited a comment on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells
Posted by GitBox <gi...@apache.org>.
XenoAmess edited a comment on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-661078806
@elharo
> Absent measurement, it can't be claimed that a PR improves performance;
OK, if you see it this way I will not oppose.
But I thought changes I made in JavadocUtil.java is easy to be think a performance refine.
> and measuring these things is hard.
Indeed. A good usecase for performance test can be very hard to built...
----------------------------------------------------------------
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-javadoc-plugin] elharo commented on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells
Posted by GitBox <gi...@apache.org>.
elharo commented on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-674892736
Running it through Jenkins now: https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-javadoc-plugin/job/MJAVADOC-653/
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443126772
##########
File path: src/test/java/org/apache/maven/plugins/javadoc/FixJavadocMojoTest.java
##########
@@ -22,10 +22,7 @@
import java.io.File;
import java.io.IOException;
import java.io.StringReader;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.List;
-import java.util.Properties;
+import java.util.*;
Review comment:
@elharo done.
----------------------------------------------------------------
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-javadoc-plugin] elharo commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443133717
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
Then try with resources isn't working right here somehow. You shouldn't need to catch IOException that's only thrown on autoclose.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443130579
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
##########
@@ -629,36 +650,31 @@ protected static String parseJavadocMemory( String memory )
throw new IllegalArgumentException( "The memory could not be null." );
}
- Pattern p = Pattern.compile( "^\\s*(\\d+)\\s*?\\s*$" );
- Matcher m = p.matcher( memory );
+ Matcher m = PARSE_JAVADOC_MEMORY_PATTERN_0.matcher( memory );
if ( m.matches() )
{
return m.group( 1 ) + "m";
}
- p = Pattern.compile( "^\\s*(\\d+)\\s*k(b)?\\s*$", Pattern.CASE_INSENSITIVE );
- m = p.matcher( memory );
+ m = PARSE_JAVADOC_MEMORY_PATTERN_1.matcher( memory );
Review comment:
@elharo done.
btw, this reusing is by original codes, and I don't like the reuse either.
now fixed.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443129951
##########
File path: src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java
##########
@@ -173,20 +173,13 @@ private static String readFile( File file )
{
String strTmp;
Review comment:
@elharo done.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-674888272
> Yes, if you could rebase and squash your commits that would be wonderful. Thanks.
@elharo
rebase done.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443130157
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
@elharo no other resource will throw IOException here.
`public URL getResource(String name) {`
IOException is not a RuntimeException. If It can be thrown it must imply it.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443137471
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
@elharo done.
----------------------------------------------------------------
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-javadoc-plugin] slachiewicz commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
slachiewicz commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r454672648
##########
File path: src/test/java/org/apache/maven/plugins/javadoc/JavadocJarTest.java
##########
@@ -154,7 +154,7 @@ public void testInvalidDestdir()
//check if the javadoc jar file was generated
File generatedFile = new File( getBasedir(),
"target/test/unit/javadocjar-invalid-destdir/target/javadocjar-invalid-destdir-javadoc.jar" );
- assertTrue( !FileUtils.fileExists( generatedFile.getAbsolutePath() ) );
+ assertFalse(FileUtils.fileExists(generatedFile.getAbsolutePath()));
Review comment:
please check here: https://maven.apache.org/developers/conventions/code.html
----------------------------------------------------------------
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-javadoc-plugin] elharo commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443121454
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractFixJavadocMojo.java
##########
@@ -856,6 +831,23 @@ private void parseClirrTextOutputFile( File clirrTextOutputFile )
}
}
+ private void methodAdded( String[] split )
+ {
+ String[] splits2;
+ List<String> list = clirrNewMethods.get( split[2].trim() );
+ if ( list == null )
+ {
+ list = new ArrayList<>();
+ }
+ splits2 = StringUtils.split( split[3].trim(), "'" );
Review comment:
initialize this here
##########
File path: src/test/java/org/apache/maven/plugins/javadoc/FixJavadocMojoTest.java
##########
@@ -22,10 +22,7 @@
import java.io.File;
import java.io.IOException;
import java.io.StringReader;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.List;
-import java.util.Properties;
+import java.util.*;
Review comment:
I'm not sure if apache maven has a style rule about this or not, but I know at Google we don't use wildcard imports.
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
##########
@@ -629,35 +650,34 @@ protected static String parseJavadocMemory( String memory )
throw new IllegalArgumentException( "The memory could not be null." );
}
- Pattern p = Pattern.compile( "^\\s*(\\d+)\\s*?\\s*$" );
+ Pattern p = PARSE_JAVADOC_MEMORY_PATTERN_0;
Matcher m = p.matcher( memory );
if ( m.matches() )
{
return m.group( 1 ) + "m";
}
-
- p = Pattern.compile( "^\\s*(\\d+)\\s*k(b)?\\s*$", Pattern.CASE_INSENSITIVE );
+ p = PARSE_JAVADOC_MEMORY_PATTERN_1;
m = p.matcher( memory );
if ( m.matches() )
{
return m.group( 1 ) + "k";
}
- p = Pattern.compile( "^\\s*(\\d+)\\s*m(b)?\\s*$", Pattern.CASE_INSENSITIVE );
+ p = PARSE_JAVADOC_MEMORY_PATTERN_2;
Review comment:
can you just use the constants here rather than assigning and reusing a local variable?
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443136388
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
@elharo
> There's something I'm not seeing here. Have you tried simply removing the catch block completely? Does that compile?
after removing that part:
```
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 12.622 s
[INFO] Finished at: 2020-06-20T22:46:09+08:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project maven-javadoc-plugin: Compilation failure
[ERROR] /D:/workspace/maven-javadoc-plugin/src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java:[6068,30] unreported exception java.io.IOException; must be caught or declared to be thrown
[ERROR] exception thrown from implicit call to close() on resource variable 'javadocClassLoader'
[ERROR]
[ERROR] -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
```
----------------------------------------------------------------
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-javadoc-plugin] elharo commented on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells
Posted by GitBox <gi...@apache.org>.
elharo commented on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-661071504
Absent measurement, it can't be claimed that a PR improves performance; and measuring these things is hard.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement; add travis-ci script.
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r436295692
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractFixJavadocMojo.java
##########
@@ -808,20 +808,6 @@ private void parseClirrTextOutputFile( File clirrTextOutputFile )
switch ( code )
{
case 7011:
Review comment:
@elharo done.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement; add travis-ci script.
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-640108640
> The Travis CI should be pulled out to a separate issue and PR.
@elharo done. commits about travis-ci has been rebased out.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r457360924
##########
File path: src/test/java/org/apache/maven/plugins/javadoc/JavadocJarTest.java
##########
@@ -154,7 +154,7 @@ public void testInvalidDestdir()
//check if the javadoc jar file was generated
File generatedFile = new File( getBasedir(),
"target/test/unit/javadocjar-invalid-destdir/target/javadocjar-invalid-destdir-javadoc.jar" );
- assertTrue( !FileUtils.fileExists( generatedFile.getAbsolutePath() ) );
+ assertFalse(FileUtils.fileExists(generatedFile.getAbsolutePath()));
Review comment:
> please check here: https://maven.apache.org/developers/conventions/code.html
@slachiewicz Thanks.
btw is there anybody for more reviewings to this pr?
----------------------------------------------------------------
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-javadoc-plugin] elharo commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443133139
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
##########
@@ -1815,7 +1831,7 @@ private static CloseableHttpClient createHttpClient( Settings settings, URL url
builder.setUserAgent( "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)" );
// Some server reject requests that do not have an Accept header
- builder.setDefaultHeaders( Arrays.asList( new BasicHeader( HttpHeaders.ACCEPT, "*/*" ) ) );
+ builder.setDefaultHeaders( Collections.singletonList( new BasicHeader( HttpHeaders.ACCEPT, "*/*" ) ) );
Review comment:
The API accepts a list, not an immutable list, and does not guarantee that it will not try to mutate that list.
----------------------------------------------------------------
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-javadoc-plugin] elharo commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443135150
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
There's something I'm not seeing here. Have you tried simply removing the catch block completely? Does that compile?
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess edited a comment on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells
Posted by GitBox <gi...@apache.org>.
XenoAmess edited a comment on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-661078806
@elharo
> Absent measurement, it can't be claimed that a PR improves performance;
OK, if you see it this way I will not oppose.
> and measuring these things is hard.
Indeed. A good usecase for performance test can be very hard to built...
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-661070559
> Can you put numbers on the performance improvement?
@elharo sorry but I didn't get what you mean by "put numbers on".
If you need performance test then I think I can try make some jmh tests.
But if you need real-time usecases performance test then I can't help with that.
----------------------------------------------------------------
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-javadoc-plugin] elharo commented on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells
Posted by GitBox <gi...@apache.org>.
elharo commented on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-676583350
Please file a JIRA issue for each actual bug.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443133786
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
##########
@@ -1815,7 +1831,7 @@ private static CloseableHttpClient createHttpClient( Settings settings, URL url
builder.setUserAgent( "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)" );
// Some server reject requests that do not have an Accept header
- builder.setDefaultHeaders( Arrays.asList( new BasicHeader( HttpHeaders.ACCEPT, "*/*" ) ) );
+ builder.setDefaultHeaders( Collections.singletonList( new BasicHeader( HttpHeaders.ACCEPT, "*/*" ) ) );
Review comment:
@elharo fine, will revert this.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-676588153
> Please file a JIRA issue for each actual bug.
OK.
----------------------------------------------------------------
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-javadoc-plugin] elharo commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement; add travis-ci script.
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r436294565
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractFixJavadocMojo.java
##########
@@ -808,20 +808,6 @@ private void parseClirrTextOutputFile( File clirrTextOutputFile )
switch ( code )
{
case 7011:
Review comment:
fallthrough is dangerous; perhpas extrat a helper method instead
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
##########
@@ -629,35 +650,34 @@ protected static String parseJavadocMemory( String memory )
throw new IllegalArgumentException( "The memory could not be null." );
}
- Pattern p = Pattern.compile( "^\\s*(\\d+)\\s*?\\s*$" );
+ Pattern p = PARSE_JAVADOC_MEMORY_PATTERN_0;
Review comment:
renaming this MEMORY_PATTERN_N does not help, arguably it makes this worse by making one look up the pattern
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443133786
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
##########
@@ -1815,7 +1831,7 @@ private static CloseableHttpClient createHttpClient( Settings settings, URL url
builder.setUserAgent( "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)" );
// Some server reject requests that do not have an Accept header
- builder.setDefaultHeaders( Arrays.asList( new BasicHeader( HttpHeaders.ACCEPT, "*/*" ) ) );
+ builder.setDefaultHeaders( Collections.singletonList( new BasicHeader( HttpHeaders.ACCEPT, "*/*" ) ) );
Review comment:
@elharo fine, will revert this.
done.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess edited a comment on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells
Posted by GitBox <gi...@apache.org>.
XenoAmess edited a comment on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-676557809
@elharo
Thanks.
As the codes are clean now, I can continue to the next step.
The next pr will be some fix about some bugs in javadoc:fix .
I will try to do it as fast as possible.
Will create pr when I finish.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443129301
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
@elharo This IOException is ignored in the original codes, so I don't think we shall rethrow it...
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443134231
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
> Then try with resources isn't working right here somehow. You shouldn't need to catch IOException that's only thrown on autoclose.
@elharo
Well, kind of agreed, but that is oracle guys' dicision for forcing us to must catch it...
Or should we revert it back to normal try instead of try-with-resource?
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
> Then try with resources isn't working right here somehow. You shouldn't need to catch IOException that's only thrown on autoclose.
@elharo
Well, kind of agreed, but that is oracle guys' dicision for forcing us to must catch it...
Or should we revert it back to normal try instead of try-with-resource?
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443133256
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
@elharo It seems only can throw when closing the `javadocClassLoader`. the original codes igonre it in `finally`. the new codes ignore it in `catch`. same thing IMO.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443126154
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractFixJavadocMojo.java
##########
@@ -856,6 +831,23 @@ private void parseClirrTextOutputFile( File clirrTextOutputFile )
}
}
+ private void methodAdded( String[] split )
+ {
+ String[] splits2;
+ List<String> list = clirrNewMethods.get( split[2].trim() );
+ if ( list == null )
+ {
+ list = new ArrayList<>();
+ }
+ splits2 = StringUtils.split( split[3].trim(), "'" );
Review comment:
@elharo done.
----------------------------------------------------------------
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-javadoc-plugin] elharo commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443133007
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
There's no catch block in the old code. Maybe this isn't thrown at all?
----------------------------------------------------------------
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-javadoc-plugin] elharo commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443129912
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
The ignored IOException was from closing in the finally block, not from other sources.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443129775
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
Review comment:
@elharo done.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-674848442
@elharo
Hi.
What should we do next for this pr?
Should I sqruash all commits now?
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443130157
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
@elharo no other source will throw IOException here.
`public URL getResource(String name) {`
IOException is not a RuntimeException. If It can be thrown it must be implied.
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443130718
##########
File path: src/test/java/org/apache/maven/plugins/javadoc/JavadocJarTest.java
##########
@@ -154,7 +154,7 @@ public void testInvalidDestdir()
//check if the javadoc jar file was generated
File generatedFile = new File( getBasedir(),
"target/test/unit/javadocjar-invalid-destdir/target/javadocjar-invalid-destdir-javadoc.jar" );
- assertTrue( !FileUtils.fileExists( generatedFile.getAbsolutePath() ) );
+ assertFalse(FileUtils.fileExists(generatedFile.getAbsolutePath()));
Review comment:
@elharo done.
btw, is there a code formatter / style checker for this code for this format we 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-javadoc-plugin] XenoAmess commented on pull request #48: [MJAVADOC-653] fix javadoc; fix code smells
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#issuecomment-661078806
> Absent measurement, it can't be claimed that a PR improves performance;
OK, if you see it this way I will not oppose.
> and measuring these things is hard.
Indeed. A good usecase for performance test can be very hard to built...
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443129539
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
##########
@@ -1815,7 +1831,7 @@ private static CloseableHttpClient createHttpClient( Settings settings, URL url
builder.setUserAgent( "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)" );
// Some server reject requests that do not have an Accept header
- builder.setDefaultHeaders( Arrays.asList( new BasicHeader( HttpHeaders.ACCEPT, "*/*" ) ) );
+ builder.setDefaultHeaders( Collections.singletonList( new BasicHeader( HttpHeaders.ACCEPT, "*/*" ) ) );
Review comment:
> Immutability makes Collections.singletonList not semantically equivalent to Arrays.asList
Yes but I don't think a immutable collection used here can be any problem.
I see no change to this collection after passed in as parameter here(in the codes).
Or please give me an example where can it break.
----------------------------------------------------------------
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-javadoc-plugin] elharo commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443137126
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
Weird. I still don't see why. For now though, just leave the code as it was since autocloseable isn't helping
----------------------------------------------------------------
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-javadoc-plugin] elharo commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443127531
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
Review comment:
pull this and the return into the try block
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
##########
@@ -629,36 +650,31 @@ protected static String parseJavadocMemory( String memory )
throw new IllegalArgumentException( "The memory could not be null." );
}
- Pattern p = Pattern.compile( "^\\s*(\\d+)\\s*?\\s*$" );
- Matcher m = p.matcher( memory );
+ Matcher m = PARSE_JAVADOC_MEMORY_PATTERN_0.matcher( memory );
if ( m.matches() )
{
return m.group( 1 ) + "m";
}
- p = Pattern.compile( "^\\s*(\\d+)\\s*k(b)?\\s*$", Pattern.CASE_INSENSITIVE );
- m = p.matcher( memory );
+ m = PARSE_JAVADOC_MEMORY_PATTERN_1.matcher( memory );
Review comment:
in general, we should avoid reusing local variables for different objects
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
##########
@@ -6066,23 +6066,15 @@ private URL getResource( final List<String> classPath, final String resource )
getLog().error( "MalformedURLException: " + e.getMessage() );
}
}
-
- URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null );
- try
+ URL resourceURL = null;
+ try ( URLClassLoader javadocClassLoader = new URLClassLoader( urls.toArray( new URL[urls.size()] ), null ) )
{
- return javadocClassLoader.getResource( resource );
- }
- finally
+ resourceURL = javadocClassLoader.getResource( resource );
+ } catch ( IOException e )
Review comment:
DANGER: this must be rethrown, not ignored
##########
File path: src/test/java/org/apache/maven/plugins/javadoc/AggregatorJavadocReportTest.java
##########
@@ -173,20 +173,13 @@ private static String readFile( File file )
{
String strTmp;
Review comment:
could pull this into the try block or even the loop initializer
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
##########
@@ -1815,7 +1831,7 @@ private static CloseableHttpClient createHttpClient( Settings settings, URL url
builder.setUserAgent( "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)" );
// Some server reject requests that do not have an Accept header
- builder.setDefaultHeaders( Arrays.asList( new BasicHeader( HttpHeaders.ACCEPT, "*/*" ) ) );
+ builder.setDefaultHeaders( Collections.singletonList( new BasicHeader( HttpHeaders.ACCEPT, "*/*" ) ) );
Review comment:
stet. Immutability makes Collections.singletonList not semantically equivalent to Arrays.asList
##########
File path: src/test/java/org/apache/maven/plugins/javadoc/JavadocJarTest.java
##########
@@ -154,7 +154,7 @@ public void testInvalidDestdir()
//check if the javadoc jar file was generated
File generatedFile = new File( getBasedir(),
"target/test/unit/javadocjar-invalid-destdir/target/javadocjar-invalid-destdir-javadoc.jar" );
- assertTrue( !FileUtils.fileExists( generatedFile.getAbsolutePath() ) );
+ assertFalse(FileUtils.fileExists(generatedFile.getAbsolutePath()));
Review comment:
Maven style uses spaces before ) and after ( unless there are no arguments
----------------------------------------------------------------
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-javadoc-plugin] XenoAmess commented on a change in pull request #48: [MJAVADOC-653] fix javadoc; fix code smells; performance improvement
Posted by GitBox <gi...@apache.org>.
XenoAmess commented on a change in pull request #48:
URL: https://github.com/apache/maven-javadoc-plugin/pull/48#discussion_r443129539
##########
File path: src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
##########
@@ -1815,7 +1831,7 @@ private static CloseableHttpClient createHttpClient( Settings settings, URL url
builder.setUserAgent( "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)" );
// Some server reject requests that do not have an Accept header
- builder.setDefaultHeaders( Arrays.asList( new BasicHeader( HttpHeaders.ACCEPT, "*/*" ) ) );
+ builder.setDefaultHeaders( Collections.singletonList( new BasicHeader( HttpHeaders.ACCEPT, "*/*" ) ) );
Review comment:
@elharo
> Immutability makes Collections.singletonList not semantically equivalent to Arrays.asList
Yes but I don't think a immutable collection used here can be any problem.
I see no change to this collection after passed in as parameter here(in the codes).
Or please give me an example where can it break.
----------------------------------------------------------------
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