Simplify JavadocStyleCheck

[rnveach]

JavadocStyleCheck currently does 5 things, which are duplicates of other checks.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocStyleCheck.java

Lines 326 to 349 in 7c693cc

	private void checkComment(final DetailAST ast, final TextBlock comment) {
	if (comment == null) {
	// checking for missing docs in JavadocStyleCheck is not consistent
	// with the rest of CheckStyle... Even though, I didn't think it
	// made sense to make another check just to ensure that the
	// package-info.java file actually contains package Javadocs.
	if (getFileContents().inPackageInfo()) {
	log(ast.getLineNo(), MSG_JAVADOC_MISSING);
	}
	}
	else {
	if (checkFirstSentence) {
	checkFirstSentenceEnding(ast, comment);
	}
	if (checkHtml) {
	checkHtmlTags(ast, comment);
	}
	if (checkEmptyJavadoc) {
	checkJavadocIsNotEmpty(comment);
	}
	}
	}

1. Ensure package-info.java has a Javadoc.
2. Checks the first sentence ends with a period and doesn't contain a specific fragement.
3. Checks that the javadoc isn't empty.
4. Checks that the tags have respective close for an open.
5. Checks that only allowed tags are used.

1 is a duplicate of MissingJavadocPackage (Since 8.22).
2 is a duplicate of SummaryJavadoc (since 6.0) but doesn't have a scope restriction.
3 is a duplicate of MissingJavadocMethod (since 8.21) and MissingJavadocType (since 8.20).
4 and 5 are the only things unique to this check.

I say we start new issues to remove the duplicate functionality, and provide the missing functionality to the other checks. This will help identify what is left for this check to be converted to an AbstractJavadocCheck.

[romani]

@rnveach ,

4. Checks that the tags have respective close for an open.
   4 is the only thing unique to this check.

I think that all antlr based checks do this, as it is requirement of proper parsing.
if this is true , we can simply remove this Check, and make migration note on what Check(configs) to use instead.

[rnveach]

@romani I assume you are referring to https://checkstyle.sourceforge.io/writingjavadocchecks.html#Tight-HTML_rules .

This violation is disabled by default. I don't know the validation JavadocStyleCheck does versus tight html to know if they are the same or not. I can definitely run regression to see if they are as another issue. If they do happen to be the same, or better, we could keep the check around and have it enable tight violation as the only thing it does.

Are you ok with me to proceed with everything else and start issues for each one?

[romani]

yes , https://checkstyle.org/config_javadoc.html#SummaryJavadoc , property violateExecutionOnNonTightHtml is disabled by default. But we can ask users to activate it if they care.

I don't know the validation JavadocStyleCheck does versus tight html to know if they are the same or not

please briefly take a look. It is kind does not matter .... we will not support custom parsing of HTML ... so if any diff exists it does not matter that much.

Are you ok with me to proceed with everything else and start issues for each one?

if you prove that usage violateExecutionOnNonTightHtml=true is better/same we simply need to remove Check, I am not sure why might need separate issues.

[rnveach]

2 ... SummaryJavadoc (since 6.0) but doesn't have a scope restriction.

Discussed offline and we will not add this property to SummaryJavadoc. If you write a javadoc, you should add a summary to it no matter it's scope.

If tight html does same validation as current JavadocStyleCheck (4) we will just remove the check itself.

[rnveach]

@romani I updated the first post. Check does 5 things instead of 4. One of the extra things it does is check if there are any tags listed that are not allowed.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocStyleCheck.java

Lines 209 to 218 in 7c693cc

	private static final Set<String> ALLOWED_TAGS = Collections.unmodifiableSortedSet(
	Arrays.stream(new String[] {
	"a", "abbr", "acronym", "address", "area", "b", "bdo", "big",
	"blockquote", "br", "caption", "cite", "code", "colgroup", "dd",
	"del", "div", "dfn", "dl", "dt", "em", "fieldset", "font", "h1",
	"h2", "h3", "h4", "h5", "h6", "hr", "i", "img", "ins", "kbd",
	"li", "ol", "p", "pre", "q", "samp", "small", "span", "strong",
	"style", "sub", "sup", "table", "tbody", "td", "tfoot", "th",
	"thead", "tr", "tt", "u", "ul", "var", })
	.collect(Collectors.toCollection(TreeSet::new)));

As for tight html, it does not look exactly the same as this check.
Example: http://rveach.no-ip.org/checkstyle/regression/reports/288/checkstyle/index.html#A552
I think tight html is very strict about everything that opens is closed. JavadocStyleCheck allows some tags to be ignored, like <p>.

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocStyleCheck.java

Lines 200 to 202 in 7c693cc

	private static final Set<String> SINGLE_TAGS = Collections.unmodifiableSortedSet(
	Arrays.stream(new String[] {"br", "li", "dt", "dd", "hr", "img", "p", "td", "tr", "th", })
	.collect(Collectors.toCollection(TreeSet::new)));

Since the 5th validation of this check isn't covered by anything else that I see, I don't think we can completely remove the check unless you want to split 4 and 5 into their own, separate checks.

I am still for removing things 1 by 1 in a single issue.