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 |
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 <[hidden email]>: > 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 |
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 <[hidden email]>: > 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 <[hidden email]>: > > 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 |
Agree with Ivan - internal code does not need mandatory Javadoc.
Most of it is meaningless and does not bring any value, just wastes everyone's time. Alexey, I think public APIs should always have Javadoc, even if it is the same thing as the member name, but with spaces - this will make the documentation nicer and easier to search. On Mon, Apr 19, 2021 at 5:21 PM Alexey Kukushkin <[hidden email]> 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 <[hidden email]>: > > > 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 <[hidden email] > >: > > > 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 > |
In reply to this post by Alexey Kukushkin
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 <[hidden email]> 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 <[hidden email]>: > > > 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 <[hidden email] > >: > > > 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 |
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 <[hidden email]> 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 < > [hidden email]> 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 <[hidden email]>: >> >> > 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 < >> [hidden email]>: >> > > 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 |
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 <[hidden email]> 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 <[hidden email]> > 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 < > > [hidden email]> 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 <[hidden email]>: > >> > >> > 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 < > >> [hidden email]>: > >> > > 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 |
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 <[hidden email]> 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 > <[hidden email]> 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 <[hidden email]> > > 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 < > > > [hidden email]> 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 <[hidden email]>: > > >> > > >> > 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 < > > >> [hidden email]>: > > >> > > 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 |
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 <[hidden email]> 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 <[hidden email]> 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 > > <[hidden email]> 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 <[hidden email]> > > > 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 < > > > > [hidden email]> 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 <[hidden email]>: > > > >> > > > >> > 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 < > > > >> [hidden email]>: > > > >> > > 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 |
Free forum by Nabble | Edit this page |