Hi, Unfortunately, such things as the "many private APIs" rule can't be formalized for any validation tool. So we have to choose between rules like "everything should be documented" and "private API documentation is not mandatory".
The community prefers the second approach. I don't want to argue about it (although I prefer to document the private API ). My goal is getting a consistent and customizable way to validate javadoc where it is mandatory (public API). It also could be applied to private API, but it should be another rule set which doesn't require javadoc but validates existing javadoc. On Wed, Jun 16, 2021 at 11:32 PM Nikita Ivanov <niva...@gridgain.com> wrote: > > Hi, > In my strong opinion Javadoc is a code. Any errors in Javadoc are > equal to the bug in the code and must be treated as such. Proper and > extensive Javadoc for all public and many private APIs is absolutely > essential for this project, its growth and maturity. > > I'm surprised this community still needs to debate fundamental > engineering issues like that... > -- > Nikita Ivanov > > On Wed, Jun 16, 2021 at 4:47 AM Andrey Gura <ag...@apache.org> wrote: > > > > Hi, > > > > I think that scope should be limited by public API for beginning. > > Also I don't sure that we should support specific tags like @apiNote, > > @implSpec, @implNote. > > > > Let's move using the following plan: > > > > 1. Create an empty (effectively) checkstyle config for javadoc > > checking and commit it to the repository. After this step it will be > > possible to configure TC in order to use this configuration. > > 2. Configure TC. > > 3. Commit a non-empty checkstyle config, but all modules should be > > excluded from checking on this step. > > 4. For each module create a JIRA issue. Module maintainers fix > > corresponding tickets and remove module exclusion from checking. > > 5. Add information about javadoc validation targets to DEVNOTES.md > > file (could be done on step 3) > > > > Any objections? > > > > On Tue, Apr 20, 2021 at 11:40 AM Andrey Mashenkov > > <andrey.mashen...@gmail.com> wrote: > > > > > > 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
