Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [tycho-dev] Change in tycho/org.eclipse.tycho.extras[master]: 425021 initial import of doc bundle mojo

Hello Jan,

this is the first time I am working with Gerrit in review mode. So
please tell me if I am doing it wrong.

I went through the comments and fixed the source for most comments,
uploaded a new patch set (actually two, but #2 still contained a wrong
space, triggering a change).

I converted the annotations from java annotations to javadoc
annotations, I not sure that I got it 100% right, that is why I wanted
to add a test setup as well, but I did not get it running. So I removed
those files before creating the patch. And from the wiki I did not find
a pointer to some explanation about running maven projects as unit test.
But I will check the module you mentioned. I peeked into the p2-extras
module, and the eclipserunner, but was not able to replicate what they did.

I also peeked into the getClasspath method, which sound better than my
approach. But I am not sure it this leads to the same results. So I will
try to create a test case first and then make the change. Maybe it would
be possible to extract that method from the abstract base class in order
to reuse it in the doc bundle mojo?!

Thanks

Jens

On 01/08/2014 11:37 AM, Jan Sievers (Code Review) wrote:
> Jan Sievers has posted comments on this change.
>
> Change subject: 425021 initial import of doc bundle mojo
> ......................................................................
>
>
> Patch Set 1: Code-Review-1
>
> (16 comments)
>
> thanks for the contribution. This is a good start but before can merge it we need at least a basic integration test (see module tycho-its) that would also document how this plugin is typically used.
> Also see my various comments inline.
>
> ....................................................
> File tycho-document-bundle-plugin/.gitignore
> Line 1: /bin
> please just copy .gitignore as well as jdt ui and core .prefs from another module. When doing this, also make sure to reformat all sources so we have a formatting consistent with the rest of the tycho sources.
>
>
> ....................................................
> File tycho-document-bundle-plugin/src/main/java/org/eclipse/tycho/extras/docbundle/JavadocMojo.java
> Line 32:  * application from the command line. In addition it creates a read to include
> ready
>
>
> Line 33:  * toc-xml file for the Eclipse Help system. <br/>
> maybe add a reference to http://help.eclipse.org/kepler/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Freference%2Fextension-points%2Forg_eclipse_help_toc.html
>
>
> Line 45:  * @author Jens Reimann
> rather add this to the contributors section in the copyright header
>
>
> Line 51:      * @parameter expression="${outputDirectory}", default-value="${project.build.directory}/reference/api", required=true 
> comma-separated syntax ofr mojo javadoc annotations is unusual, not sure if it works. Normally these are space-separated. See http://maven.apache.org/developers/mojo-api-specification.html
>
>
> Line 57:      * @parameter expression="${basedir}", required=true, editable=false
> normally 
> @required
> @readonly
>
> are used.
>
>
> Line 90:      * @parameter default-value="compile,provided", expression="${scopes}"
> not sure if comma-separated default-values and expressions are atomatically parsed into multiple values.
>
>
> Line 104:     private JavadocOptions javadocOptions = new JavadocOptions ();
> AFAIK deeply nested (XML) mojo parameters cannot be parsed from String expressions
>
>
> Line 108:      * @parameter expression="${tocOptions}"
> AFAIK deeply nested (XML) mojo parameters cannot be parsed from String expressions
>
>
> Line 115:      * @parameter required=false, default-value="${project.build.directory}/tocjavadoc.xml", expression="${tocFile}"
> required=false is the default
>
>
> Line 124:     public void setTocOptions ( TocOptions tocOptions )
> If you don't use them in the code, setters are not required for dependency-injected mojo fields so you could probably remove them.
>
>
> Line 271:     private class GatherClasspathVisitor implements ProjectVisitor
> not sure but if you want to be tycho-specific here, you could use the classpath already computed by tycho.
> See AbstractOsgiCompilerMojo.getClassPath() for an example how to do this.
>
>
> Line 279:                 if ( dep.getSystemPath () != null )
> why filter only system-scoped dependencies? maybe it's better/ more consistent to use the classpath computed by tycho which is also used for compilation (see my comment above).
>
>
> ....................................................
> File tycho-document-bundle-plugin/src/main/java/org/eclipse/tycho/extras/docbundle/TocWriter.java
> Line 147:         final String result = base.toURI ().relativize ( file.toURI () ).getPath ();
> canonicalize both files before calling toURI() . E.g. there can be uppercase/lowercase drive letter differences on Windows which would make the URI String comparison during relativize() fail.
>
>
> ....................................................
> File tycho-eclipserun-plugin/.settings/org.eclipse.jdt.core.prefs
> Line 2
> unrelated change which should not be included
>
>
> ....................................................
> File tycho-version-bump-plugin/.settings/org.eclipse.jdt.core.prefs
> Line 1: eclipse.preferences.version=1
> I guess this change is unrelated and should not be included
>
>


-- 
IBH SYSTEMS GmbH
D-85235 Pfaffenhofen an der Glonn
Läutenring 43
Geschäftsführer / CEO: Dr. Thomas Heitzig

Amtsgericht München
Handelsregister Nummer  HRB 197959
USt ID: DE267945175

Office Munich
D 80992 München
Agnes-Pockels-Bogen 1
T +49 89 18 9 17 49 0

The information transmitted is intended only for the person or entity
to which it is addressed and may contain confidential and/or pivileged
material. Any review, retransmission, dissemination or other use of,
or taking of any action in reliance upon, this information by persons
or entities other than the intended recipient is prohibited. If you
received this in error, please contact the sender and delete the
material from any computer.



Back to the top