Hi Benjamin, in other words, anything we have @Deprecated annotation on top of (or anything you want to annotate with it). Does it help with the explanation?
For the initial phase, I plan to just put "since" everywhere (into every already existing @Deprecated annotation) and we leave out "forRemoval" in Deprecated annotation for now as that is quite tricky to get right. I am confused what is considered to be removed and what we keep there for ever even it is deprecated (referring to what Mick said in this thread that forRemoval can not be by default true). After we map what technical debt we have, we can summarize this and I bring it to the ML again for further discussion what to actually remove and when. Regards ________________________________________ From: Benjamin Lerer <b.le...@gmail.com> Sent: Friday, October 13, 2023 12:19 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. 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<mailto: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<mailto: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<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcheckstyle.sourceforge.io%2Fapidocs%2Fcom%2Fpuppycrawl%2Ftools%2Fcheckstyle%2Fchecks%2Fannotation%2FMissingDeprecatedCheck.html&data=05%7C01%7CStefan.Miklosovic%40netapp.com%7C59fa2b3786ff436c83ba08dbcbd5ece7%7C4b0911a0929b4715944bc03745165b3a%7C0%7C0%7C638327891917050879%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=8qKu8ob%2BvPdHfUQdkxr5C%2BgkR5iMcUaEqw9a%2FNN276k%3D&reserved=0> > > > > ________________________________________ > > From: Francisco Guerrero <fran...@apache.org<mailto:fran...@apache.org>> > > Sent: Tuesday, October 10, 2023 23:34 > > To: dev@cassandra.apache.org<mailto: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<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcheckstyle.sourceforge.io%2Fapidocs%2Fcom%2Fpuppycrawl%2Ftools%2Fcheckstyle%2Fchecks%2Fannotation%2FMissingDeprecatedCheck.html&data=05%7C01%7CStefan.Miklosovic%40netapp.com%7C59fa2b3786ff436c83ba08dbcbd5ece7%7C4b0911a0929b4715944bc03745165b3a%7C0%7C0%7C638327891917050879%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=8qKu8ob%2BvPdHfUQdkxr5C%2BgkR5iMcUaEqw9a%2FNN276k%3D&reserved=0> > > > [2] > > > https://checkstyle.org/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/MatchXpathCheck.html<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcheckstyle.org%2Fapidocs%2Fcom%2Fpuppycrawl%2Ftools%2Fcheckstyle%2Fchecks%2Fcoding%2FMatchXpathCheck.html&data=05%7C01%7CStefan.Miklosovic%40netapp.com%7C59fa2b3786ff436c83ba08dbcbd5ece7%7C4b0911a0929b4715944bc03745165b3a%7C0%7C0%7C638327891917050879%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=U6MIohIvPO%2FyGZIHoATZRxwfnvuiyaCtGaBzz0bqw1k%3D&reserved=0> > > > > > > On Tue, 10 Oct 2023 at 14:50, Josh McKenzie > > > <jmcken...@apache.org<mailto: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<mailto:m...@apache.org>> > > > > Sent: Monday, October 9, 2023 10:55 > > > > To: dev@cassandra.apache.org<mailto: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. > > > > > > > > > > > >
