Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace missing-doclet with a maintained, configurable dependency on the Central Repository #4541

Open
dbwiddis opened this issue Sep 18, 2022 · 4 comments
Labels
Build Libraries & Interfaces enhancement Enhancement or improvement to existing feature or request

Comments

@dbwiddis
Copy link
Member

Is your feature request related to a problem? Please describe.

I spent a few hours yesterday adding some javadocs to the example plugins to chip away at #221.

The vast majority of the fixes I had to do were trivial (probably biased by the "example" nature of that particular code). There were many updates required to obvious getters, which results in things like this:

/**
 * Gets the foo.
 *
 * @return the foo
 */
public Foo getFoo() {
    return this.foo;
}

/**
 * Sets the foo.
 *
 * @param foo the foo to set
 */
public void setFoo(Foo foo) {
    this.foo = foo;
}

I can understand those who still want to require these, and perhaps make them more verbose than what I posted. But what really annoyed me was the fact that as of JDK-18 (which I have installed), the strict Javadoc lint check requires a javadoc on the default (no-arg) constructor, even if it doesn't exist. Multiple times I had to take code like this...

/**
 * The Foo class does things.
 */
public class Foo extends SuperFoo {
    @Override
    barMethod() { ... }
}

... and actually add the default constructor just so I could javadoc it! This is completely useless for a no-arg constructor, there are no @params to document, and the class itself already requires a javadoc so anything in the constructor would be redundant.

/**
 * The Foo class does things.
 */
public class Foo extends SuperFoo {

    /*
     * Instantiate this object.
     */
    public Foo() {}

    @Override
    barMethod() { ... }
}

There should be a configuration to not require these trivial Javadocs.

  • Some IDE linters have this as an option in their checks, e.g., IntelliJ IDEA's "Declaration has Javadoc problems" has a checkbox to "Ignore simple Property Accessors".
  • Checkstyle has an option for MissingJavadocMethodCheck to allowMissingPropertyJavadoc if it matches the format above.

A few months ago I added javadoc checks to the opensearch-sdk-java repo. My first attempt was to simply implement the same system as OpenSearch, using the doc-tools module. This proved to be frustrating:

  • The doc-tools module is not published any repository, so other repos do not have access to it. They have to manually copy all of the files (including three build.gradle files) along with copying and customizing the missing-javadoc configuration file in the /gradle directory of the main project. This is a lot of duplication just to use the same tool.
  • The current approach is not very configurable. You can only set the level (down to package level granularity, but with exact names, no wildcarding).

I looked into possibly editing the code to avoid having to add default constructors just to document them, but realized I'd only be editing my copy and not pushing the change upstream (here) so others could take advantage. I wondered where this code came from... seems to be from Solr:

// Original version of this file is ported from the render-javadoc code in Lucene,
// original code under the Apache Software Foundation under Apache 2.0 license
// See - https://github.com/apache/lucene-solr/tree/master/gradle/documentation/render-javadoc

The link is dead because Lucene and Solr split their repos but the latest code is here and also duplicated here, with a few fixes since we copied it. But neither Lucene nor Solr publishes it to the Central Repository either. At least they keep up with each other, but they are only two interrelated projects. OpenSearch has dozens of repos.

There has got to be a better way.

Describe the solution you'd like

Imagine if some project had the same capability as the missing-doclet code, was well-maintained, was configurable, and was published to the Central Repository.

No need to imagine, there is: https://github.com/plume-lib/require-javadoc

Did it have the custom configurations I wanted when I first found it? Nope, but I submitted an issue and the maintainer added them within a day or two, and published a new version. Adding it was insanely simple, just another dependency in build.gradle, along with the custom configuration I wanted. Package-level (with wildcard) exclusions are possible, along with some other bells and whistles. See a sample implementation here.

Describe alternatives you've considered
Checkstyle is an alternative, but is very heavyweight if this is the only feature we're using from it (and spotless handles most of the other requirements we would implement.)

Additional context
"One of my most productive days was throwing away 1,000 lines of code." -- Ken Thompson.

@dbwiddis dbwiddis added enhancement Enhancement or improvement to existing feature or request untriaged labels Sep 18, 2022
@saratvemulapalli
Copy link
Member

I like the idea, I do not know the details but I understand we'd like to remove duplicate piece of code and replace it with 3P plugin.
@dblock @reta @nknize thoughts?

@reta
Copy link
Collaborator

reta commented Jan 25, 2023

I like the idea, I do not know the details but I understand we'd like to remove duplicate piece of code and replace it with 3P plugin. @dblock @reta @nknize thoughts?

@saratvemulapalli totally missed your comment, apologies, looking into it now

@reta
Copy link
Collaborator

reta commented Jan 26, 2023

@saratvemulapalli @dbwiddis I have been looking into this issue for a couple of days, here is what we have with respect to missing javadocs checks:

  • it indeed came from Solr / Lucene (at some point in the past) and it is still there: https://github.com/apache/lucene/tree/main/dev-tools/missing-doclet
  • the same for gradle/missing-javadoc.gradle, copied from https://github.com/apache/lucene/blob/main/gradle/documentation/render-javadoc.gradle (at some point in the past)
  • because Apache Lucene's dev-tools are not published to Apache Maven repos, well, the module was copied, along with Gradle task(s)

Now, could we use something different, like https://github.com/plume-lib/require-javadoc? I think we could but there is a catch: although some checks become easier (like default constructor), others will be missed because MissingDoclet applies very different, more strict in a sense, rules.

Basically, to summarize in a few words - OpenSearch copies Apache Lucene approach with respect to mandating javadocs presence. Replacing that with anything else would work for a price of loosing the overall checks strictness (plus the features we use currently are specific to MissingDoclet and would require additional work to add more javadocs).

I think that's the trade-off we have, @dblock @nknize do you guys have some thoughts on the subject?
Thank you.

@dbwiddis
Copy link
Member Author

Quick update. We ended up removing requireJavadoc from SDK as it's most useful for JDK 17 and earlier, and wasn't very helpful with the new javadoc rules in JDK 18+.

However, Checkstyle is a well-maintained program, has a gradle plugin that makes installing it a one-liner, and is very configurable with fine-grained regex-based rules that I'm pretty sure would be able to duplicate the approach here, if not improve it.

And yes, I know checkstyle does a lot more style checking that we've removed and I'm not suggesting we use it for that. Just for javadocs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Libraries & Interfaces enhancement Enhancement or improvement to existing feature or request
Projects
None yet
Development

No branches or pull requests

4 participants