You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Andrey Mashenkov <an...@gmail.com> on 2021/04/19 10:30:03 UTC
[DISCUSSION] Javadoc style requirements and checks in Ignite-3.
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
Re: [DISCUSSION] Javadoc style requirements and checks in Ignite-3.
Posted by Andrey Mashenkov <an...@gmail.com>.
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 <ni...@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
> > > <an...@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.mashenkov@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 <
> > > > > kukushkinalexey@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 <vololo100@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.mashenkov@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
Re: [DISCUSSION] Javadoc style requirements and checks in Ignite-3.
Posted by Andrey Gura <ag...@apache.org>.
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 <ni...@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
> > <an...@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 <an...@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 <
> > > > kukushkinalexey@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 <vo...@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.mashenkov@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
Re: [DISCUSSION] Javadoc style requirements and checks in Ignite-3.
Posted by Nikita Ivanov <ni...@gridgain.com>.
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
> <an...@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 <an...@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 <
> > > kukushkinalexey@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 <vo...@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.mashenkov@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
Re: [DISCUSSION] Javadoc style requirements and checks in Ignite-3.
Posted by Andrey Gura <ag...@apache.org>.
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
<an...@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 <an...@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 <
> > kukushkinalexey@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 <vo...@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.mashenkov@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
Re: [DISCUSSION] Javadoc style requirements and checks in Ignite-3.
Posted by Andrey Mashenkov <an...@gmail.com>.
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 <an...@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 <
> kukushkinalexey@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 <vo...@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.mashenkov@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
Re: [DISCUSSION] Javadoc style requirements and checks in Ignite-3.
Posted by Andrey Mashenkov <an...@gmail.com>.
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 <ku...@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 <vo...@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.mashenkov@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
Re: [DISCUSSION] Javadoc style requirements and checks in Ignite-3.
Posted by Pavel Tupitsyn <pt...@apache.org>.
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 <ku...@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 <vo...@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.mashenkov@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
>
Re: [DISCUSSION] Javadoc style requirements and checks in Ignite-3.
Posted by Alexey Kukushkin <ku...@gmail.com>.
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 <vo...@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 <an...@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
Re: [DISCUSSION] Javadoc style requirements and checks in Ignite-3.
Posted by Ivan Pavlukhin <vo...@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 <an...@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