On 2023/10/11 16:59:35 Maxim Muzafarov wrote:
> Francisco,
>
> I agree with your vision of the deprecation comments and actually, I
> think we should recommend doing it that way for the cases where it is
> applicable on our code-style page, but when things get to the
> implementation phase there are some obstacles that are not easy to
> overcome.
Yeah, I agree that this should be recommended rather than enforced via
some checkstyle rule. However, reviewers should be aware of this
recommendation in the code-style page.
>
> So, adding the MissingDeprecated will emphasize to a developer the
> need to describe the deprecation reasons in comments, but
> unfortunately, there is no general pattern that we can enforce for
> every such description message and/or automatically validate its
> meaningfulness. There may be no alternative for a deprecated field, or
> it may simply be marked for deletion, so the pattern is slightly
> different in this case.
+1 for adding the MissingDeprecated rule
> Another problem is how to add meaningful comments to the deprecated
> annotations that we already have in the code, since we can't enforce
> checkstyle rules only on newly added code. This is a very exhausting
> process with no 100% guarantee of accuracy - some of the commits don't
> have a good commit message and require a deep archaeology.
Not aiming for 100% accuracy, but more on code style agreement.
> All of the above led me to the following which is pretty easy to
> achieve and improves the code quality:
>
> /** @deprecated See CASSANDRA-6504 */
> @Deprecated(since = "2.1")
> public Integer concurrent_replicates = null;
>
> On Wed, 11 Oct 2023 at 09:51, Miklosovic, Stefan
> <stefan.mikloso...@netapp.com> wrote:
> >
> > Here (1) it supports check of both Javadoc and annotation at the same time
> > so what you want is possible. What is not possible is to checkstyle the
> > _content_ of deprecated Javadoc nor any format of it. I think that ensuring
> > the presence of both annotation and Javadoc comment is just enough.
> >
> > (1)
> > https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/annotation/MissingDeprecatedCheck.html
> >
> > ________________________________________
> > From: Francisco Guerrero <fran...@apache.org>
> > Sent: Tuesday, October 10, 2023 23:34
> > To: dev@cassandra.apache.org
> > Subject: Re: [DISCUSS] putting versions into Deprecated annotations
> >
> > NetApp Security WARNING: This is an external email. Do not click links or
> > open attachments unless you recognize the sender and know the content is
> > safe.
> >
> >
> >
> >
> > To me this seems insufficient. As a developer, I'd like to see what the
> > alternative is when reading the javadoc without having to go to Jira.
> >
> > What I would prefer is to know what the alternative is and how to use it.
> > For example:
> >
> > /** @deprecated Use {@link #alternative} instead. See CASSANDRA-6504 */
> > @Deprecated(since = "2.1")
> > public Integer concurrent_replicates = null;
> >
> > I am not sure if checkstyle can enforce the above, so the mechanisms to
> > enforce it would still need to be laid out, unless we can easily support
> > something like the above with checkstyle rules.
> >
> > On 2023/10/10 20:34:27 Maxim Muzafarov wrote:
> > > Hello everyone,
> > >
> > >
> > > I've discussed with Stefan some steps we can take to improve the final
> > > solution, so the final version might look like this:
> > >
> > > /** @deprecated See CASSANDRA-6504 */
> > > @Deprecated(since = "2.1")
> > > public Integer concurrent_replicates = null;
> > >
> > > The issue number will be taken from the git blame comment. I doubt I
> > > can generate and/or create a meaningful comment for every deprecation
> > > annotation, but providing a link to the issue that was retrieved from
> > > the git blame is not too big a problem. This also improves the
> > > visibility.
> > >
> > > In addition, we can add two checkstyle rules [1] [2] to ensure that
> > > any future deprecations will have a "since" element and a JavaDoc
> > > comment.
> > > WDYT?
> > >
> > > [1]
> > > https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/annotation/MissingDeprecatedCheck.html
> > > [2]
> > > https://checkstyle.org/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/MatchXpathCheck.html
> > >
> > > On Tue, 10 Oct 2023 at 14:50, Josh McKenzie <jmcken...@apache.org> wrote:
> > > >
> > > > Sounds like we're relitigating the basics of how @Deprecated,
> > > > forRemoval, since, and javadoc @link all intersect to make deprecation
> > > > less painful ;)
> > > >
> > > > So:
> > > >
> > > > Built-in java.lang.Deprecated: required
> > > > Can use since and forRemoval if you have that info handy and think it'd
> > > > be useful (would make it a lot easier to grep for things to pull before
> > > > a major)
> > > > If it's being replaced by something, you should {@link #} the javadoc
> > > > for it so people know where to bounce over to
> > > >
> > > > I've been leaning pretty heavily on the functionality of point 3 for
> > > > documenting cross-module implicit dependencies as I come across them
> > > > lately so that one resonates with me.
> > > >
> > > > On Tue, Oct 10, 2023, at 4:38 AM, Miklosovic, Stefan wrote:
> > > >
> > > > OK.
> > > >
> > > > Let's go with in-built java.lang.Deprecated annotation. If somebody
> > > > wants to document that in more detail, there are Javadocs as mentioned.
> > > > Let's just stick with the standard stuff.
> > > >
> > > > I will try to implement this for 5.0 (versions since it was deprecated)
> > > > with my take on what should be removed (forRemoval = true) but that
> > > > should be definitely cross-checked on review as Mick mentioned.
> > > >
> > > > ________________________________________
> > > > From: Mick Semb Wever <m...@apache.org>
> > > > Sent: Monday, October 9, 2023 10:55
> > > > To: dev@cassandra.apache.org
> > > > Subject: Re: [DISCUSS] putting versions into Deprecated annotations
> > > >
> > > > NetApp Security WARNING: This is an external email. Do not click links
> > > > or open attachments unless you recognize the sender and know the
> > > > content is safe.
> > > >
> > > >
> > > >
> > > > Tangential question to this is if everything we deprecated is eligible
> > > > for removal? In other words, are there any cases when forRemoval would
> > > > be false? Could you elaborate on that and give such examples or do you
> > > > all think that everything which is deprecated will be eventually
> > > > removed?
> > > >
> > > >
> > > > Removal cannot be default. This came up in the subtickets of
> > > > CASSANDRA-18306.
> > > >
> > > > I suggest that adding " forRemoval = true" and the later actual removal
> > > > of the code both require broader consensus. I'm open to that being on
> > > > the ticket or needing a thread on the ML. Small stuff, common sense
> > > > says on the ticket is enough, but a few folk have already stated that
> > > > deprecated code that has minimal maintenance overhead should not be
> > > > removed.
> > > >
> > > >
> > >
>
