Hi Andrey, I've update a PR [1]. * Added maven task for Javadoc style checks (public API only, for now) * Updated DEVNOTES. * Suppress Javadoc style errors for broken modules using Checkstyle suppression configuration. Unfortunately, maven plugin configuration allows to filter out module files/subdirs only, but not the entire module. So, I've used the checkstyle configuration for this purpose.
Can someone create an additional TC task for this? [1] https://github.com/apache/ignite-3/pull/98 On Thu, Jun 17, 2021 at 12:06 PM Andrey Gura <ag...@apache.org> wrote: > 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 > -- Best regards, Andrey V. Mashenkov
