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 2022/08/18 18:43:56 UTC
[GitHub] [maven-plugin-tools] kwin opened a new pull request, #139: MPLUGIN-417 Descriptor with plain text by default
kwin opened a new pull request, #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139
Report uses enhanced descriptor with HTML
Include better links to javadoc
Include more block taglets in description
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r957604381
##########
maven-plugin-plugin/src/main/java/org/apache/maven/plugin/plugin/AbstractGeneratorMojo.java:
##########
@@ -43,12 +42,6 @@
@Parameter( defaultValue = "${project}", readonly = true )
protected MavenProject project;
- /**
- * The goal prefix that will appear before the ":".
- */
- @Parameter
- protected String goalPrefix;
Review Comment:
I agree, will back out those unrelated changes.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin commented on pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin commented on PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#issuecomment-1220665623
> The commit is too big for me to see what's happening.
Right now you can completely ignore the newly added classes, just look at the modification of the existing ones, to tell me what you think of the approach in general.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin commented on pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin commented on PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#issuecomment-1220658423
> IIRC in the past I've made a change that the two outputs are always based on the same source: the plugin descriptor.
The whole point is that this approach prevents emitting the full details. Currently the plugin descriptor contains inline taglets (which is wrong) and the HTML report does no longer have access to the full context. So in order for the report to not go via
`(parse source file information -> convert javadoc to html -> convert html to plaintext -> serialize descriptor) -> (deserialize descriptor -> convert plain text to html)`
(while the former block being goal `descriptor` and the latter being `report`) you have to do the parsing twice.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r956150516
##########
maven-plugin-tools-java/src/main/java/org/apache/maven/tools/plugin/extractor/javadoc/JavaJavadocMojoDescriptorExtractor.java:
##########
@@ -56,11 +56,13 @@
/**
* <p>
Review Comment:
Do we need to convert javadoc -> XHTML for the legacy extractor as well?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r956553530
##########
pom.xml:
##########
@@ -94,9 +94,13 @@
<javaVersion>8</javaVersion>
<pluginTestingHarnessVersion>3.3.0</pluginTestingHarnessVersion>
<mavenVersion>3.2.5</mavenVersion>
+ <!-- SLF4J version must match the version exported from the mavenVersion,
+ https://github.com/apache/maven/blob/12a6b3acb947671f09b81f49094c53f426d8cea1/pom.xml#L63 -->
Review Comment:
Not sure, as even minor versions may introduce new features which are backwards-compatible (i.e. new methods added to API). When you leverage those your code breaks when running with an older SLF4J API dependency.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin merged pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin merged PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] slawekjaranowski commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r954954561
##########
maven-plugin-plugin/pom.xml:
##########
@@ -319,6 +319,9 @@
<maven.compiler.source>${maven.compiler.source}</maven.compiler.source>
<maven.compiler.target>${maven.compiler.target}</maven.compiler.target>
</properties>
+ <pomExcludes>
+ <pomExclude>plugin-info-jdk/pom.xml</pomExclude><!-- relies on just a given plugin.xml for generating a report, this is no longer supported -->
Review Comment:
maybe remove 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] rfscholte commented on pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
rfscholte commented on PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#issuecomment-1220800807
I think I get your point regarding the context. Based on that it is indeed fine to use HTML as content for the plugin descriptor. Just make sure there's a test that requires this context, as it is the critical part of this solution.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] slawekjaranowski commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r956426452
##########
maven-plugin-plugin/src/main/java/org/apache/maven/plugin/plugin/AbstractGeneratorMojo.java:
##########
@@ -43,12 +42,6 @@
@Parameter( defaultValue = "${project}", readonly = true )
protected MavenProject project;
- /**
- * The goal prefix that will appear before the ":".
- */
- @Parameter
- protected String goalPrefix;
Review Comment:
It is need:
https://github.com/apache/maven-plugin-tools/blob/706b1d0b6730d028350f18d8459eee8b123e2f67/maven-plugin-tools-generators/src/main/resources/help-class-source.vm#L44
Without it HelpMojo.java will have unresolved value
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r956314886
##########
maven-plugin-plugin/src/main/java/org/apache/maven/plugin/plugin/AbstractGeneratorMojo.java:
##########
@@ -43,12 +42,6 @@
@Parameter( defaultValue = "${project}", readonly = true )
protected MavenProject project;
- /**
- * The goal prefix that will appear before the ":".
- */
- @Parameter
- protected String goalPrefix;
Review Comment:
goalPrefix is almost useless in helpmojo. It is just used for the report on this mojo. Will try to remove this last remnant
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r996961032
##########
maven-script/maven-plugin-tools-beanshell/src/main/java/org/apache/maven/tools/plugin/extractor/beanshell/BeanshellMojoDescriptorExtractor.java:
##########
@@ -131,6 +131,7 @@ private MojoDescriptor createMojoDescriptor( String basedir, String resource, Pl
throw new InvalidPluginDescriptorException( "Error scanning beanshell script", evalError );
}
+ // FIXME: convert javadocs
Review Comment:
I left the legacy handling in place, i.e. convert inline javadoc tags afterwards to HTML (but only for legacy extractors).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin commented on pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin commented on PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#issuecomment-1219818889
@rfscholte What do you think of this approach and scanning the sources twice for plain-text and html separately?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] slawekjaranowski commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r956563467
##########
maven-plugin-plugin/src/main/java/org/apache/maven/plugin/plugin/AbstractGeneratorMojo.java:
##########
@@ -43,12 +42,6 @@
@Parameter( defaultValue = "${project}", readonly = true )
protected MavenProject project;
- /**
- * The goal prefix that will appear before the ":".
- */
- @Parameter
- protected String goalPrefix;
Review Comment:
It is needed for this change?
If no - can we move such change to separate 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] slawekjaranowski commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r956255139
##########
maven-plugin-plugin/src/main/java/org/apache/maven/plugin/plugin/AbstractGeneratorMojo.java:
##########
@@ -43,12 +42,6 @@
@Parameter( defaultValue = "${project}", readonly = true )
protected MavenProject project;
- /**
- * The goal prefix that will appear before the ":".
- */
- @Parameter
- protected String goalPrefix;
Review Comment:
Why you move this parameter connected stuff to `DescriptorGeneratorMojo`
We lost goalPrefix in HelpGeneratorMojo ?
##########
maven-plugin-plugin/src/main/java/org/apache/maven/plugin/plugin/report/PluginReport.java:
##########
@@ -195,11 +197,22 @@
* Path to {@code plugin.xml} plugin descriptor to generate the report from.
*
* @since 3.5.1
+ * @deprecated No longer evaluated, use {@link
Review Comment:
unfinished ... docs
##########
maven-plugin-plugin/src/main/java/org/apache/maven/plugin/plugin/AbstractGeneratorMojo.java:
##########
@@ -88,44 +81,7 @@ public void execute()
return;
}
- if ( !"maven-plugin".equalsIgnoreCase( project.getArtifactId() )
- && project.getArtifactId().toLowerCase().startsWith( "maven-" )
- && project.getArtifactId().toLowerCase().endsWith( "-plugin" )
- && !"org.apache.maven.plugins".equals( project.getGroupId() ) )
- {
- getLog().error( LS + LS + "Artifact Ids of the format maven-___-plugin are reserved for" + LS
- + "plugins in the Group Id org.apache.maven.plugins" + LS
- + "Please change your artifactId to the format ___-maven-plugin" + LS
- + "In the future this error will break the build." + LS + LS );
- }
Review Comment:
Similar - is not valid for both goals descriptor and help?
##########
pom.xml:
##########
@@ -94,9 +94,13 @@
<javaVersion>8</javaVersion>
<pluginTestingHarnessVersion>3.3.0</pluginTestingHarnessVersion>
<mavenVersion>3.2.5</mavenVersion>
+ <!-- SLF4J version must match the version exported from the mavenVersion,
+ https://github.com/apache/maven/blob/12a6b3acb947671f09b81f49094c53f426d8cea1/pom.xml#L63 -->
Review Comment:
IMHO the same major is 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r956550981
##########
maven-plugin-plugin/src/main/java/org/apache/maven/plugin/plugin/AbstractGeneratorMojo.java:
##########
@@ -43,12 +42,6 @@
@Parameter( defaultValue = "${project}", readonly = true )
protected MavenProject project;
- /**
- * The goal prefix that will appear before the ":".
- */
- @Parameter
- protected String goalPrefix;
Review Comment:
As I said: " is almost useless". This only affects the comments on the class which is evaluated by report. I would rather get rid of the prefix in the comment and and just reference the full Maven coordinates here. WDYT?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r956151146
##########
maven-script/maven-plugin-tools-beanshell/src/main/java/org/apache/maven/tools/plugin/extractor/beanshell/BeanshellMojoDescriptorExtractor.java:
##########
@@ -131,6 +131,7 @@ private MojoDescriptor createMojoDescriptor( String basedir, String resource, Pl
throw new InvalidPluginDescriptorException( "Error scanning beanshell script", evalError );
}
+ // FIXME: convert javadocs
Review Comment:
necessary to convert javadocs -> xhtml for deprecated BSH extractor?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r956313386
##########
maven-plugin-plugin/src/main/java/org/apache/maven/plugin/plugin/AbstractGeneratorMojo.java:
##########
@@ -88,44 +81,7 @@ public void execute()
return;
}
- if ( !"maven-plugin".equalsIgnoreCase( project.getArtifactId() )
- && project.getArtifactId().toLowerCase().startsWith( "maven-" )
- && project.getArtifactId().toLowerCase().endsWith( "-plugin" )
- && !"org.apache.maven.plugins".equals( project.getGroupId() ) )
- {
- getLog().error( LS + LS + "Artifact Ids of the format maven-___-plugin are reserved for" + LS
- + "plugins in the Group Id org.apache.maven.plugins" + LS
- + "Please change your artifactId to the format ___-maven-plugin" + LS
- + "In the future this error will break the build." + LS + LS );
- }
Review Comment:
Logging it more than once is a bit redundant and you always need to run descriptor after helpmojo anyways. Therefore just logging in descriptor seems to be enough
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] kwin commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r956314886
##########
maven-plugin-plugin/src/main/java/org/apache/maven/plugin/plugin/AbstractGeneratorMojo.java:
##########
@@ -43,12 +42,6 @@
@Parameter( defaultValue = "${project}", readonly = true )
protected MavenProject project;
- /**
- * The goal prefix that will appear before the ":".
- */
- @Parameter
- protected String goalPrefix;
Review Comment:
goalPrefix is almost useless in helpmojo. It is just used for the html report on this mojo. Will try to remove this last remnant
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] slawekjaranowski commented on a diff in pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on code in PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#discussion_r956423748
##########
maven-plugin-tools-java/src/main/java/org/apache/maven/tools/plugin/extractor/javadoc/JavaJavadocMojoDescriptorExtractor.java:
##########
@@ -56,11 +56,13 @@
/**
* <p>
Review Comment:
I would not touch legacy extractors
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-plugin-tools] rfscholte commented on pull request #139: MPLUGIN-417 Descriptor with plain text by default
Posted by GitBox <gi...@apache.org>.
rfscholte commented on PR #139:
URL: https://github.com/apache/maven-plugin-tools/pull/139#issuecomment-1220650598
The commit is too big for me to see what's happening.
With TDD I'd like to see:
- plugin descriptor keeps the original javadoc, so inlcluding inline doclettags.
- the generated help-goal prints plain text
- the report generator of the goal transforms the inline doclettags to valid html.
IIRC in the past I've made a change that the two outputs are always based on the same source: the plugin descriptor.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org