I am a bit confused by the starting point of this discussion: "When we deprecate APIs / methods" What are we exactly calling APIs/methods? It is really unclear to me what we are talking about here.
Le jeu. 12 oct. 2023 à 02:38, Francisco Guerrero <fran...@apache.org> a écrit : > > > 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. > > > > > > > > > > > > > > > > >
