I would be OK with failing the build for Javadoc warnings, and having a single cleanup pass to fix this. I think the kinds of issues we have (mismatching names/parameters) are the least of our documentation problems though.
I think it would be great to introduce guidance for authoring Javadoc, but I haven’t given much thought how to express best practices here, or even what best practice looks like. I suspect our biggest documentation problem is a matter of incentives and priorities more than guidance though. There are some things we could perhaps do to improve matters in that respect, such as requiring top level interfaces to have Javadoc on every method and failing the build if they do not (with some easy way to disable it, to avoid low quality boilerplate Javadoc to silence the warning), but I would suggest we open a separate thread to consider this topic. From: Stefan Miklosovic <stefan.mikloso...@instaclustr.com> Date: Tuesday, 31 May 2022 at 14:20 To: dev@cassandra.apache.org <dev@cassandra.apache.org> Subject: Re: Updating our Code Contribution/Style Guide Hi Benedict, all, I do not want to hijack this thread, if we want to have separate discussion about that I can open one but anyway ... What do you think about Javadocs? I do not see that it is mentioned but Javadocs are technically the code as well (well, they are written in the source code, right?). We have a lot of Javadoc warnings / errors when one wants to build it. What I noticed is that the build reports at most 100 of Javadoc issues but there is _way more_ of them. I already started to clean it up but after a while I realized it is too big of a change to do during "rainy afternoon" and I was not even sure what branch I should target. So, do we want to target cleanup of Javadocs? I think we should target just the trunk. Do you think we need any formal documentation / guidance as to how to write Javadocs? Anything specific? I think that we should strive for having 0 issues on Javadocs and flat out error the build if some are found. If we do not seek any significant improvement in this area (as a community), I would like to have it explicitly stated. Regards On Tue, 31 May 2022 at 14:46, bened...@apache.org <bened...@apache.org> wrote: > > I think that it is hard to define what the right extent of a patch is, but it > should be the minimal scope that the author feels sufficient to safely > address the concerns of the patch. I have added a sentence to this effect in > the top section of the proposal. > > > > My view (not propagated to the document) is that we should generally as a > rule avoid pure mechanistic clean-up work, unless it is associated with an > important refactor (and hence, likely to be trunk only). I would normally > give the cleanup its own commits for review, but not at merge. > > > > We don’t currently have any project norms around linter warnings, only errors > that we enforce with checkstyle and ecj. So I think right now it’s down to > personal taste at commit time, as part of any patch-related cleanup. > > > > Do we want to try pursuing zero warnings for commits by some linters? This > might be a good thing, if we are willing to be liberal with @SupressWarnings. > I’m not sure how we would transition, though, with so many existing warnings. > > > > > > > > From: Ekaterina Dimitrova <e.dimitr...@gmail.com> > Date: Monday, 30 May 2022 at 21:37 > To: dev@cassandra.apache.org <dev@cassandra.apache.org> > Subject: Re: Updating our Code Contribution/Style Guide > > I also like it, thank you for putting it together. We can always add more and > more, but I think the current one is already quite extensive. I like the > dependency management point. > > > > I want to clarify a bit only one point. Any kind of old warnings and code > cleaning. If it is not immediately related to the patch, we should do those > in trunk and if it requires a lot of noise - probably in a separate > commit/ticket, no? Is this a valid statement? I've seen different opinions > but I feel it is good to have a consensus and this feels like a good time to > mention it. I mean cases where there are classes with 20 warnings, etc and > they may exist since early versions for example. > > > > Best regards, > > Ekaterina > > > > On Mon, 30 May 2022 at 14:10, Derek Chen-Becker <de...@chen-becker.org> wrote: > > Looks great! > > > > On Mon, May 30, 2022, 5:37 AM bened...@apache.org <bened...@apache.org> wrote: > > Any more feedback around this? Everyone happy with the latest proposal? > > > > From: bened...@apache.org <bened...@apache.org> > Date: Sunday, 15 May 2022 at 15:08 > To: dev@cassandra.apache.org <dev@cassandra.apache.org> > Subject: Re: Updating our Code Contribution/Style Guide > > I agree with this sentiment, but I think it will require a bit of time to > figure out where that balance is. > > > > I’ve inserted a mention of @Nullable, @ThreadSafe, @NotThreadSafe and > @Immutable. > > > > > If we only use one of the two - for example @Nullable - that leaves us with > > "We know the original author expected this to be null at some point in its > > lifecycle and it means something" and "We have no idea if this is legacy > > and nullable or not" > > > > My inclination is to start building some norms around this, carefully as we > don’t have enough experience and understanding of the pitfalls and long term > usage. But, my preferred norms would be that properties should be assumed to > be @Nonnull and that all nullable parameters and properties should be marked > as @Nullable. This is how I use these properties today; Nonnull always seems > superfluous, as it is rare to have a set of properties where null is the > default, or where it is particularly important that the reader or compiler > realise this. > > > > There will be an interim period, in particular for legacy code, where this > may lead to less clarity. But in the long term this is probably preferable to > inconsistent usage where some areas of the codebase indicate @Nonnull without > indicating @Nullable, and vice-versa, or where every variable and method ends > up marked with one or the other. > > > > This is probably also most consistent with a future world of cheap Optional > types (i.e. Valhalla), where Nullable may begin to be replaced with Optional, > and Nonnull may become very much the default. > > > > That said, as stated multiple times, the author and reviewer’s determinations > are final. This document just sets up some basic parameters/expectations. > > > > From: Derek Chen-Becker <de...@chen-becker.org> > Date: Saturday, 14 May 2022 at 20:56 > To: dev@cassandra.apache.org <dev@cassandra.apache.org> > Subject: Re: Updating our Code Contribution/Style Guide > > On Sat, May 14, 2022 at 11:00 AM Josh McKenzie <jmcken...@apache.org> wrote: > > > > Incidentally, I've found similar value in @ThreadSafe, const, readonly, etc - > communications of author's intent; being able to signal to future maintainers > helps them make modifications that are more consistent with and safer with > regards to the original intention and guarantees of the author. > > > > Assuming you trust those guarantees that is. :) > > > > I think author's intent is important, which is why I also think that > judicious/effective commenting and naming are important (and I'm glad that > naming is called out in the guidelines explicitly). However, I also think > that these are also opportunities to help the compiler and tooling help us, > similar to how Benedict's draft calls out effective use of the type system as > a way to encode semantics and constraints in the code. These annotations, > while clunky and verbose, do open the door in some cases to static analysis > that the Java compiler is incapable of doing. I don't know exactly where it > is, but I think there's a balance between use of annotations to help tooling > identify problems while not becoming onerous for current and future > contributors. I know this is more difficult in Java than, say, Rust, but I'm > an eternal optimist and I think we can find that balance :) > > > > Cheers, > > > > Derek > > > > -- > > +---------------------------------------------------------------+ > > | Derek Chen-Becker | > > | GPG Key available at https://keybase.io/dchenbecker and | > > | https://pgp.mit.edu/pks/lookup?search=derek%40chen-becker.org | > > | Fngrprnt: EB8A 6480 F0A3 C8EB C1E7 7F42 AFC5 AFEE 96E4 6ACC | > > +---------------------------------------------------------------+ > >
