I've fixed the PR. Now, 1. Javadoc style is only checked if it is present for the element. All declared javadoc tags MUST have a description. So, one should either write correct javadoc or don't write at all. 2. Additional checks performed for non-internal and non-test classes. All public and protected elements must have non-emtpy javadoc.
So, checkstyle detects 500+ missed descriptions (missed javadocs, tags descriptions, tags themselves) in public scope and 180+ bad-styles (missed brased, spaces, empty-lines) javadocs in whole codebase. I'd suggest to force non-empty javadocs for all non-test classes including internal (but their members) as well. On Mon, Apr 19, 2021 at 6:37 PM Andrey Mashenkov <andrey.mashen...@gmail.com> wrote: > Alexey, > > These are bad examples and unfortunately, most of the Ignite code doesn't > look self-descriptive. > (Do you feel the difference between @SerializableTransient and > @TransientSerializable?) > > To understand the semantic of e.g. 'metricsHistSize' you have to analyze > its usages. > Getter and setter for this property look better, but it still not clear > what happens with metric values above the limit? > > I have another example, one of the core components GridDhtPartitionDemander > has totally useless javadoc. > It is obviously has nothing with thread pool + "local cache" phrase looks > very confusing. > > /** > * Thread pool for requesting partitions from other nodes and populating > local cache. > */ > public class GridDhtPartitionDemander > > To understand the purpose of this internal component I have to analyze its > code and usages. > However, if one will face unexpected behavior, he/she may be confused if > it is a lack of javadoc or wrong behavior. > > There is no way to restrict or avoid such issues with any checks. > Agree, meaningful javadocs as self-descriptive code is more about culture > and discipline. > > The absence of javadoc on internal API, unreasonably raises the entry > threshold for new developers and makes the development of intra-component > interaction harder. > I admit a high-level description for public classes may be enough to > resolve this without pushing developers to write empty/useless docs for > every method/field. > > > On Mon, Apr 19, 2021 at 5:21 PM Alexey Kukushkin < > kukushkinale...@gmail.com> wrote: > >> I personally do not understand why we need mandatory javadoc even in >> public >> modules. I think javadoc is needed only when the code is not >> self-descriptive. What value does a javadoc like this bring >> < >> https://github.com/apache/ignite/blob/2.10.0/modules/core/src/main/java/org/apache/ignite/configuration/IgniteConfiguration.java >> > >> to a developer: >> >> /** Default metrics history size (value is {@code 10000}). */ >> public static final int DFLT_METRICS_HISTORY_SIZE = 10000; >> >> /** Metrics history time. */ >> private int metricsHistSize = DFLT_METRICS_HISTORY_SIZE; >> >> BTW, I picked a random example and it already has a copy-paste mistake in >> the javadoc, which is there for years. I think that indicates it has no >> real value and developers just do it to comply with the rule. >> >> Some say mandatory javadoc is for the discipline but I think Apache Ignite >> developers are mature enough to understand when additional documentation >> is >> really required. >> >> >> >> пн, 19 апр. 2021 г. в 16:37, Ivan Pavlukhin <vololo...@gmail.com>: >> >> > Hi Andrey and Igniters, >> > >> > Sorry if I misread something but I have totally different opinion in >> > one aspect. In my mind it is much better in practice to follow strict >> > rules for public API javadocs but not for internals. I would use >> > static checks for API packages and disable them for internal ones and >> > refine code readability during code review. Main motivation here is >> > that ubiquitous javadocs did not work well in ignite-2 and I believe >> > it would not in ignite-3. >> > >> > 2021-04-19 13:30 GMT+03:00, Andrey Mashenkov < >> andrey.mashen...@gmail.com>: >> > > Hi Igniters, >> > > >> > > We use JDK Javadoc tool to validate javadocs on TC (Javadoc suite) in >> > > Ignite 2 and now in Ignite 3. >> > > A javadoc tool is designed for javadoc site generation that also >> performs >> > > some basic checks and markup validation, >> > > but has nothing to do with javadoc styles. >> > > >> > > I've found maven-checkstyle-plugin has modules for javadoc style >> > checking, >> > > which looks more suited for the issue. >> > > I've tried to turn on javadoc checks in checkstyle plugin, then run it >> > > against Ignite-3 main branch and got 1200+ errors. >> > > >> > > For Ignite-2 thing may goes worse and numbers can be much higher, >> > > but let please, start a separate discussion for this if one feels it >> make >> > > sense. >> > > >> > > Javadoc is part of documentation which a face of product and we MUST >> keep >> > > high standards for javadocs as well. >> > > >> > > Let's improve this within the ticket [1] regarding the next >> suggestions: >> > > 1. Add Javadoc checks to mavan-checkstyle-plugin. I've made a PR for >> > > Ignite-3 [1] to turn on style checks for javadocs. >> > > >> > > 2. Keep the current Javadoc TC suite as is. because it allow detecting >> > > markup errors regarding current javadoc tool version capabilities. >> > > >> > > 3. Fix the Codestyle guidelines to follow higher standards. >> > > 3.1. Disallow empty javadocs (or with missed description) for member >> that >> > > can be used outside the class/package/module by users or other >> > developers. >> > > I believe all methods/classes/fields that can be used by developers >> > > (public, protected, package visible) MUST have meaningful description, >> > > excepts may be tests, well-known constants (e.g. serialVersionUid), >> and >> > > private members. >> > > Actually, it not a big deal to put few words into javadoc even if the >> > > method looks simple, >> > > if one feels difficulties expressing a class/method purpose, then most >> > > likely refactoring is needed. >> > > >> > > 3.2. Check all params/throws/returns/generics/deprecates MUST be >> > > well-documented and goes in order. >> > > >> > > 3.4. Suggest to allow optional tags @apiNote, @implSpec, @implNote as >> > > described in [3], >> > > to put e.g. expectations/requirements of implementation for developers >> > that >> > > may be non-obvious. >> > > The tags values are rendered in separate blocks on javadoc site. >> > > >> > > 3.5 However, one-liner javadoc like "{@inheritDoc}" does nothing and >> can >> > be >> > > safely omitted. I'd keep it. >> > > >> > > 3.6 Add javadoc checks for 'package-info'. Do we want an additional >> > > requirement to every package has package-info? >> > > >> > > Working on the patch I've found it is impossible to have different >> > policies >> > > in the same config for different scopes: source and test code. >> > > Thus, we can either exclude tests from style checks at all, which >> looks >> > > like not a good idea, >> > > or have different configs with different policies for source and test >> > code. >> > > >> > > Any thoughts? >> > > >> > > [1] https://issues.apache.org/jira/browse/IGNITE-14591 >> > > [2] https://github.com/apache/ignite-3/pull/98 >> > > [3] http://openjdk.java.net/jeps/8068562 >> > > >> > > -- >> > > Best regards, >> > > Andrey V. Mashenkov >> > > >> > >> > >> > -- >> > >> > Best regards, >> > Ivan Pavlukhin >> > >> >> >> -- >> Best regards, >> Alexey >> > > > -- > Best regards, > Andrey V. Mashenkov > -- Best regards, Andrey V. Mashenkov
