Fix javadoc errors

[dr0i]

Doing ./gradlew install fails with javadoc errors resulting in a failed build.
This PR fixes the javadoc errors.

That make me think about changing github actions to not only do a run: ./gradlew check but a full run: ./gradlew install - or do I miss something @blackwinter ?

[blackwinter]

That make me think about changing github actions to not only do a run: ./gradlew check but a full run: ./gradlew install

I'd suggest enabling the JavadocMethod Checkstyle rule instead:

diff --git config/checkstyle/checkstyle.xml config/checkstyle/checkstyle.xml
index a5415c3d..f10d9268 100644
--- config/checkstyle/checkstyle.xml
+++ config/checkstyle/checkstyle.xml
@@ -68,6 +68,7 @@
         <module name="InterfaceIsType"/>
         <module name="InterfaceTypeParameterName"/>
         <module name="JavaNCSS"/>
+        <module name="JavadocMethod"/>
         <module name="JavadocType"/>
         <module name="LambdaParameterName"/>
         <module name="LeftCurly"/>

Which shows the following violations:

[ant:checkstyle] [ERROR] .../metafacture-biblio/src/main/java/org/metafacture/biblio/marc21/MarcXmlEncoder.java:246:40: Expected @param tag for 'str'. [JavadocMethod]
[ant:checkstyle] [ERROR] .../metafacture-biblio/src/main/java/org/metafacture/biblio/marc21/MarcXmlEncoder.java:251:44: Expected @param tag for 'str'. [JavadocMethod]
[ant:checkstyle] [ERROR] .../metafacture-xml/src/main/java/org/metafacture/xml/XmlElementSplitter.java:65:8: Unused @param tag for 'aTopLevelElement'. [JavadocMethod]
[ant:checkstyle] [ERROR] .../metafacture-xml/src/main/java/org/metafacture/xml/XmlElementSplitter.java:66:8: Unused @param tag for 'aElementName'. [JavadocMethod]
[ant:checkstyle] [ERROR] .../metafacture-xml/src/main/java/org/metafacture/xml/XmlElementSplitter.java:68:44: Expected @param tag for 'topLevelElement'. [JavadocMethod]
[ant:checkstyle] [ERROR] .../metafacture-xml/src/main/java/org/metafacture/xml/XmlElementSplitter.java:68:74: Expected @param tag for 'elementName'. [JavadocMethod]
[ant:checkstyle] [ERROR] .../metafacture-xml/src/main/java/org/metafacture/xml/XmlElementSplitter.java:86:8: Unused @param tag for 'root'. [JavadocMethod]
[ant:checkstyle] [ERROR] .../metafacture-xml/src/main/java/org/metafacture/xml/XmlElementSplitter.java:89:49: Expected @param tag for 'newRoot'. [JavadocMethod]

[dr0i]

Ok, yes we could do this. But help me understand this:
public void omitXmlDeclaration(final boolean currentOmitXmlDeclaration) has no javadoc for @param and no check error nonetheless (although this is a public method) while private void writeRaw(final String str) errors (although that's just a private method).
However, I propose to stick to scope public:

diff --git a/config/checkstyle/checkstyle.xml b/config/checkstyle/checkstyle.xml
index a5415c3d..c7b63a98 100644
--- a/config/checkstyle/checkstyle.xml
+++ b/config/checkstyle/checkstyle.xml
@@ -68,6 +68,9 @@
         <module name="InterfaceIsType"/>
         <module name="InterfaceTypeParameterName"/>
         <module name="JavaNCSS"/>
+        <module name="JavadocMethod">
+            <property name="scope" value="public"/>
+        </module>
         <module name="JavadocType"/>

What's your opinion?

[blackwinter]

public void omitXmlDeclaration(final boolean currentOmitXmlDeclaration) has no javadoc for @param and no check error nonetheless (although this is a public method)

This method doesn't have any Javadoc at all; the JavadocMethod rule doesn't verify its presence, that would be MissingJavadocMethod.

However, I propose to stick to scope public:

Since these are the only two violations for private methods we currently have, I'd say we just fix them and keep the default scope. But I'm fine either way.

[dr0i]

Right, thx for the hint!
ACK to keep the default scope of JavadocMethod.

So I like to have MissingJavadocMethod to ensure having the API documented. But using this results in a lot of missing java doc not only for methods but also for public constructors. Many (all?) of the latter just define a "defaul"t constructor, e.g.:

public PicaXmlHandler() {}

Are these "default constructors" (well, strictly they are not "default" - but are enhanced by the compiler to behave exactly like the default conctructor) necessary ?
From http://www.java2s.com/Tutorials/Java/OCA_Java_SE_8_Class_Design/0030__Java_Constructor_Inheritance.htm (section "Compiler Enhancements"):

... [these] class and constructor definitions are equivalent [to the compiler provided default constructor]

Couldn't we just get rid of them? (If so, this implies also removing the module MissingCtor in checkstyle.xml because it forces the appearance of a constructor (and be it only a default constructor)).

[Edit]: hm, ok, and then there is this blog post. Maybe it's better to keep MissingCtor but exclude constructors from being verified. Or we could provide a simple javadoc for all these no-arg-constructors like e.g. "just call super()" or "simple no-arg-constructor"?

[blackwinter]

It probably boils down to what we actually want, then we can adjust the config accordingly (see also #389):

1. Do we want explicit constructors that don't necessarily require Javadoc? Then enable MissingJavadocMethod but remove CTOR_DEF from tokens.
2. Do we want explicit constructors but don't require Javadoc for empty constructors? Then enable MissingJavadocMethod but set minLineCount to 1 (?).
3. Do we want Javadoc everywhere but don't insist on explicit constructors? Then enable MissingJavadocMethod and remove MissingCtor (along with all the empty constructors that were only introduced to satisfy that rule).

I'm leaning towards 2. But I'm not particularly invested in Javadoc, anyway, so I'll leave it up to you ;)

[dr0i]

You boiled it down quiet nicely :) And I also like 2 the most (see also the edit in #396 (comment)). Will go with that.

[blackwinter]

Wow, what a Herculean task! I've added some more fixes, but overall nice job :)

We should probably configure additional checks now that we're officially committed to writing Javadoc comments. But that can happen later.

Also, there are a bunch of @value references to private constants which can't be resolved:

metafacture-io/src/main/java/org/metafacture/io/ObjectWriter.java:56: warning - #STDOUT (referenced by @value tag) is an unknown reference.
metafacture-io/src/main/java/org/metafacture/io/ObjectWriter.java:56: warning - #STDOUT (referenced by @value tag) is an unknown reference.
metafacture-monitoring/src/main/java/org/metafacture/monitoring/ObjectBatchLogger.java:68: warning - #DEFAULT_FORMAT (referenced by @value tag) is an unknown reference.
metafacture-monitoring/src/main/java/org/metafacture/monitoring/ObjectBatchLogger.java:68: warning - #DEFAULT_FORMAT (referenced by @value tag) is an unknown reference.
metafacture-monitoring/src/main/java/org/metafacture/monitoring/ObjectBatchLogger.java:68: warning - #DEFAULT_FORMAT (referenced by @value tag) is an unknown reference.
metafacture-xml/src/main/java/org/metafacture/xml/GenericXmlHandler.java:62: warning - #RECORD_TAG_PROPERTY (referenced by @value tag) is an unknown reference.
metafacture-xml/src/main/java/org/metafacture/xml/GenericXmlHandler.java:93: warning - DEFAULT_RECORD_TAG (referenced by @value tag) is an unknown reference.
metamorph/src/main/java/org/metafacture/metamorph/functions/ISBN.java:101: warning - #ISBN10 (referenced by @value tag) is an unknown reference.
metamorph/src/main/java/org/metafacture/metamorph/functions/ISBN.java:101: warning - #ISBN13 (referenced by @value tag) is an unknown reference.
metamorph/src/main/java/org/metafacture/metamorph/functions/Unique.java:80: warning - #NAME (referenced by @value tag) is an unknown reference.
metamorph/src/main/java/org/metafacture/metamorph/functions/Unique.java:80: warning - #VALUE (referenced by @value tag) is an unknown reference.

We should either make them public or drop the references.

[dr0i]

Great - thx for the improvements! Noted some things.

[blackwinter]

Awesome! 🎉

I still get some Javadoc reference errors, though:

metafacture-biblio/src/main/java/org/metafacture/biblio/marc21/MarcXmlEncoder.java:136: warning - #NAMESPACE_NAME (referenced by @value tag) is an unknown reference.
metafacture-biblio/src/main/java/org/metafacture/biblio/marc21/MarcXmlEncoder.java:136: warning - #NAMESPACE_NAME (referenced by @value tag) is an unknown reference.
metafacture-biblio/src/main/java/org/metafacture/biblio/marc21/MarcXmlEncoder.java:136: warning - #NAMESPACE_NAME (referenced by @value tag) is an unknown reference.
metafacture-commons/src/main/java/org/metafacture/commons/StringUtil.java:63: warning - #DEFAULT_VARSTART (referenced by @value tag) is an unknown reference.
metafacture-commons/src/main/java/org/metafacture/commons/StringUtil.java:63: warning - #DEFAULT_VAREND (referenced by @value tag) is an unknown reference.
metafacture-commons/src/main/java/org/metafacture/commons/StringUtil.java:94: warning - #DEFAULT_VARSTART (referenced by @value tag) is an unknown reference.
metafacture-commons/src/main/java/org/metafacture/commons/StringUtil.java:94: warning - #DEFAULT_VAREND (referenced by @value tag) is an unknown reference.
metafacture-strings/src/main/java/org/metafacture/strings/StreamUnicodeNormalizer.java:157: warning - Tag @link: reference not found: Normalizer.Form
metafacture-strings/src/main/java/org/metafacture/strings/UnicodeNormalizer.java:76: warning - Tag @link: reference not found: Normalizer.Form

Fixed in 3b8289e. But I wonder why they weren't picked up by the Github actions build...

[dr0i]

I wonder why they weren't picked up by the Github actions build...

Probably a bugfix in 1.8? I use java version 1.8.0_292.

[blackwinter]

Probably a bugfix in 1.8? I use java version 1.8.0_292.

8u302 here. Github actions doesn't show the patch version.