Skip to content

Conversation

@gselzer
Copy link
Member

@gselzer gselzer commented Mar 25, 2021

This PR makes the usage of the @Parameter annotation obsolete; from now on, parameter names and descriptions should be described within the Op's Javadoc. At compile time, the Javadoc will be stored, to be parsed during runtime. As there are 3 different types of Ops, there are 3 different ways to store the parameter data:

Ops written as Methods

Intuitively, the @param and @return tags can be applied within the Op's javadoc. Note that if only some of the parameters are described, the parser will not be able to determine which parameters correspond to the given tags. In this case the parser will give up and synthesize names (and empty descriptions) for the Op. This means that an Op written as

	/**
	 * @param foo the first input
	 * @param bar the second input
	 * @return foo + bar
	 */
	@OpMethod(names = "test.javadoc.method", type = BiFunction.class)
	public static List<Long> OpMethodFoo(List<String> foo, List<String> bar) {
		BiFunction<String, String, Long> func = (s1, s2) -> Long.parseLong(s1) +
			Long.parseLong(s2);
		return Streams.zip(foo.stream(), bar.stream(), func).collect(Collectors
			.toList());
	}

will correspond to an OpInfo

public static java.util.List<java.lang.Long> org.scijava.param.JavadocParameterTest.OpMethodFoo(java.util.List<java.lang.String>,java.util.List<java.lang.String>)(
	 Inputs:
		java.util.List<java.lang.String> foo -> the first input
		java.util.List<java.lang.String> bar -> the second input
	 Outputs:
		java.util.List<java.lang.Long> output -> foo + bar
)

Ops written as Classes

Documentation for Ops written as Classes is written similarly to that of OpMethods, however the javadoc should be written on the functional method of the Op. For example:

/**
 * Test Op used to see if we can't scrape the javadoc.
 * 
 * @author Gabriel Selzer
 */
@Plugin(type = Op.class, name = "test.javadoc.class")
class JavadocOp implements Function<Double, Double> {

	/**
	 * @param t the input
	 * @return the output
	 */
	@Override
	public Double apply(Double t) {
		return t + 1;
	}

}

This Op will produce an OpInfo:

org.scijava.param.JavadocOp(
	 Inputs:
		java.lang.Double t -> the input
	 Outputs:
		java.lang.Double output -> the output
)

Ops written as Fields:

Ops written as Fields require different Javadoc syntax, as Fields do not have the @param and @return tags that Methods do (and we don't want to misuse them 😅 ). Thus we substitute @param for @input and @return for @output:

	/**
	 * @input in the input
	 * @output the output
	 */
	@OpField(names = "test.javadoc.field")
	public final Function<Double, Double> javadocFieldOp = (in) -> in + 1;

This Op produces an OpInfo:

org.scijava.param.JavadocParameterTest.javadocFieldOp(
	 Inputs:
		java.lang.Double in -> the input
	 Outputs:
		java.lang.Double output -> the output
)

Ideally, we would write Taglets for both @input and @output, however I have not (yet) had success in doing so (as the syntax changed in Java 9). This work may be pursued in a later PR.

TODO:

  • Do we pursue the Taglet work now, or at a later date (so we can get this out of the door). There is some work on an InputTaglet in this iteration of the codebase. We should move it to another branch if we decide to wait on it.
  • Clean Git History / decide to squash + merge (looking at you d2c131d)
  • Merge Data provenance mechanism #35, as this work is built on top of that work

Notes:

  • This PR introduces a dependency on therapi-runtime-javadoc. The version information should probably be moved upstream.

@gselzer gselzer force-pushed the scijava/scijava-ops/view-javadoc branch 3 times, most recently from d048699 to fe13f4b Compare March 26, 2021 17:43
@gselzer
Copy link
Member Author

gselzer commented Apr 22, 2021

To set up javadoc scraping within your own Java 11 Maven project:

  1. Depend on therapi-runtime-javadoc and javax.annotation-api within your project:
	    <dependency>
		    <groupId>javax.annotation</groupId>
		    <artifactId>javax.annotation-api</artifactId>
		    <version>1.3.1</version>
	    </dependency>
	    <dependency>
		    <groupId>com.github.therapi</groupId>
		    <artifactId>therapi-runtime-javadoc-scribe</artifactId>
		    <version>0.12.0</version>
	    </dependency>
		<dependency>
			<groupId>com.github.therapi</groupId>
			<artifactId>therapi-runtime-javadoc</artifactId>
			<version>0.12.0</version>
		</dependency>

NOTE: depending on scijava-ops will automatically register all three of these dependencies

  1. Register JavadocAnnotationProcessor within the maven compiler plugin:
<plugin>
	<groupId>org.apache.maven.plugins</groupId>
	<artifactId>maven-compiler-plugin</artifactId>
	<version>3.8.1</version>
	<configuration>
		<annotationProcessors>
			<annotationProcessor>com.github.therapi.runtimejavadoc.internal.JavadocAnnotationProcessor</annotationProcessor>
		</annotationProcessors>
		<debug>true</debug>
		<optimize>true</optimize>
		<compilerArguments>
			<AaddGeneratedAnnotation>true</AaddGeneratedAnnotation>
			<Adebug>true</Adebug>
		</compilerArguments>
		<annotationProcessorPaths>
			<path>
				<groupId>com.github.therapi</groupId>
				<artifactId>therapi-runtime-javadoc-scribe</artifactId>
				<version>0.12.0</version>
			</path>
		</annotationProcessorPaths>
	</configuration>
</plugin>

For users of JPMS, there is an additional step: You must require therapi.runtime.javadoc, report that the module uses javax.annotation.processing.Processor and open and export all packages within your module to therapi.runtime.javadoc. Thus a module foo with packages bar and baz might have the following module-info.java:

module foo {
  exports foo.bar to therapi.runtime.javadoc;
  exports foo.baz to therapi.runtime.javadoc;

  opens foo.bar to therapi.runtime.javadoc;
  opens foo.baz to therapi.runtime.javadoc;

  requires therapi.runtime.javadoc;

  uses javax.annotation.processing.Processor;
}

You must open and export to all projects you wish to scrape the javadoc of.

I have created a small example of how this works here

@ctrueden ctrueden self-requested a review April 27, 2021 17:37
Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for working on this! ✨ I am excited to see the @Parameter annotation DIAF. 😆 🔥

We discussed a bit some of the issues surrounding this work. Here are some thoughts, in no particular order:

  1. Therapi is an source-time annotation processor. It might have access to the parameter names of methods, even though they are typically not stored into the bytecode. If so, we could map the @param names to the actual parameter names, in case the @param declarations are out of order, or some are missing, or some are spurious. This would be better than the current assumption that everything is there, and in same order as the method. However, it's not a blocker for merging this PR—it can always be fixed later. The question for then is: do we need to fork therapi, or write our own annotation processor with analogous logic, or what?
  2. Relatedly, there is also the flag allowing to compile parameter names into the bytecode—we could add this to pom-scijava so that every project does this by default. It expands the bytecode a little, but in my tests, it was small (<3%? I don't remember exactly). But does Ops need it?
  3. We should test whether <scope>provided</scope> works for the therapi-related dependencies. Certainly, we do not want otherwise-independent projects such as imglib2-algorithm to become polluted with therapi runtime dependencies; it is only ops that needs to depend on it at runtime, for reading the javadoc. Relatedly, we should test in both JPMS and non-JPMS scenarios. Curtis thinks provided scope is probably not what we ultimately want, even if it works, because it's also a form of dependency pollution of sorts. What we really want is to add therapi to the annotation processor classpath. This can be done in the maven-compiler-plugin configuration. Here is an SO post about it; in a nutshell, you can add the deps to the compiler plugin's dependencies, instead of the project dependencies.
  4. In the case of mixed @param and @input/@output, therapi indexes things in such a way that the order is lost. We should consider either:
    1. Submitting an issue and/or PR to therapi adding a general-purpose getter of all the javadoc annotations in original order; or
    2. Fork therapi and merge its logic into scijava-indexer, so that everyone can have this functionality without incurring additional annotation-time dependencies.

As far as what actually needs to happen for this PR to merge:

  • Convert therapi usage to annotation processor classpath (i.e. maven-compiler-plugin classpath). Ensure it works.
  • Clean up git history.
  • File issues for any remaining concerns, as detailed above.
  • Remove support for @input/@output/etc. from OpClass and OpMethod. People need to always use @param/@return for those types of ops. Otherwise, the javadoc won't be fully correct. This also avoids dealing with issue (4) above.
  • Log a warning for any op that does not have properly javadoc'd parameters.
  • Fix the JPMS configuration: every package that we want javadoc for needs to be opened and exported to therapi. This raises a larger question of: is there automation we could introduce into pom-scijava easing the burden of module-info.java for our community? This is outside the scope of ops specifically, but would be super helpful for people.

@gselzer
Copy link
Member Author

gselzer commented May 14, 2021

[x] Fix the JPMS configuration: every package that we want javadoc for needs to be opened and exported to therapi. This raises a larger question of: is there automation we could introduce into pom-scijava easing the burden of module-info.java for our community? This is outside the scope of ops specifically, but would be super helpful for people

I solved this issue by using Apache Velocity to autogenerate the module-info. Ideally, as @ctrueden mentions above, it would be ideal for us to do this in the POM, however it is a bit of a tricky problem. The conventional method of adding opens via the POM, however, does not offer a simple solution.

  • Remove support for @input/@output/etc. from OpClass and OpMethod. People need to always use @param/@return for those types of ops. Otherwise, the javadoc won't be fully correct. This also avoids dealing with issue (4) above.

This is also fixed. Interestingly, I found out that therapi treats these as equivalent:

	/**
	 * Tests javadoc scraping of mutable taglet
	 * 
	 * @param foo the i/o argument
	 */
	/**
	 * Tests javadoc scraping of mutable taglet
	 * 
	 * @param foo the i/o argument
	 * @return
	 */

As far as I can tell, there is no way to determine whether the @return tag has been written (although Eclipse does throw a warning in the latter case). In both scenarios, a return Comment is generated and is empty, so the best available method for checking whether or not there is a return tag is to check doc.getReturn().toString.isEmpty(). This isn't quite right, but is probably fine for right now. We might want to fix this if we make our own annotation processor / fork therapi

@gselzer gselzer force-pushed the scijava/scijava-ops/view-javadoc branch 2 times, most recently from 0822a9b to 6bbcd18 Compare May 19, 2021 18:01
@gselzer
Copy link
Member Author

gselzer commented May 19, 2021

In response to the logging bullet point, I have created issue scijava/scijava#64 (as it should sit on the backburner until scijava/scijava#13 has been completed). For that reason, I will check that bullet point.

@gselzer gselzer force-pushed the scijava/scijava-ops/view-javadoc branch 7 times, most recently from 958a97c to 30f47f9 Compare May 20, 2021 16:33
@gselzer
Copy link
Member Author

gselzer commented May 20, 2021

@ctrueden as of this writing, all commits build with passing tests

One consideration for a future issue: we still use Velocity to generate the module-info (and use ModuleTest to verify that packages are opened to therapi correctly). But it would be better if we could have Maven do it for us automatically. But this is not feasible via compilerArgs alone as we'd have to hardcode in each package we want to open to a particular module. We'd want to do some research to see if a Maven plugin is capable of doing this for us. I don't think this is worth holding up the PR, though.

@gselzer gselzer force-pushed the scijava/scijava-ops/view-javadoc branch from 30f47f9 to f0b8c1c Compare June 28, 2021 18:08
@gselzer gselzer marked this pull request as ready for review July 13, 2021 19:28
@hinerm hinerm dismissed ctrueden’s stale review July 13, 2021 19:50

Changes addressed

@gselzer gselzer force-pushed the scijava/scijava-ops/view-javadoc branch from f0b8c1c to a654a04 Compare August 5, 2021 14:58
There were two different methods that wrote printouts for OpInfos, using
rather different syntaxes. We decided that one syntax was better, and
added member highlight support (which was only available using the other
    syntax).
It has outlived its usefulness to us. RIP
This change applies only to method and class Ops, however it enables
@input synonymous with @param and @output synonymous with @return. This
also allows us to use @container and @mutable on Ops written as classes
and as methods. This then changes the paradigm to make the parsing of
@param and @return a bonus, but favors using @input and @output
respectively.
Suppose that an Inplace OpMethod is annotated with the functional
interface BiFunction. We must catch these situations quickly and fail
before the struct is generated, as it is not the job of ParameterStructs
to deal with incorrectly annotated methods
Used so that we maintain one tag per input for Ops written as methods.
This taglet should not appear on Ops written as Fields or as Classes
(although it will not break anything if they do, the javadoc parser just
 ignores these lines)
Unnecessary - maven handles this for us
Not sure how these escaped the purge
The annotation processor side of therapi should not be transitively
passed to projects depending on scijava ops - we only need the side of
therapi that looks up the javadoc
This ensures that everyone will be writing proper javadoc whenever
proper javadoc will be sufficient for our purposes
They are not being used by our annotation processors, so there is no
reason to keep them.
Doing this allows us to remove the maven compiler plugin conifguration
from scijava-ops' POM
This is not needed; pom-scijava-base does this for us
When we move the module-path annotation processors upstream, we want to
make as few changes as possible. These options are not necessary, thus
we'll remove them
@ctrueden ctrueden force-pushed the scijava/scijava-ops/view-javadoc branch from 9ddc289 to 7d44ec7 Compare November 15, 2021 23:25
Because it's not a standard dependency for everything, but it needs
to be built first, so that it's available during the javadoc build.
@ctrueden ctrueden merged commit 4aba8f3 into main Nov 19, 2021
@ctrueden ctrueden deleted the scijava/scijava-ops/view-javadoc branch November 19, 2021 15:38
@ctrueden
Copy link
Member

I don't see anything else substantial to do on this PR, so I didn't even bother to start a review. My review points from earlier have already been addressed. Thanks! 💯

@ctrueden ctrueden removed their assignment Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

4 participants