Closed dr0i closed 2 years ago
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?
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.
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"?
It probably boils down to what we actually want, then we can adjust the config accordingly (see also #389):
MissingJavadocMethod
but remove CTOR_DEF
from tokens
.MissingJavadocMethod
but set minLineCount
to 1
(?).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 ;)
You boiled it down quiet nicely :) And I also like 2 the most (see also the edit in https://github.com/metafacture/metafacture-core/pull/396#issuecomment-931385139). Will go with that.
Added the missing javadoc for public methods and constructors. Changes committed are grouped by the metafacture modules. Note that the added javadoc may sometimes be more, sometimes less helpful. The main idea is that new implementations forces coders to document their API - and they are the experts who best know how to document it.
Awesome! :tada:
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...
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.
Probably a bugfix in 1.8? I use java version 1.8.0_292.
8u302
here. Github actions doesn't show the patch version.
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 fullrun: ./gradlew install
- or do I miss something @blackwinter ?