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 | > > +---------------------------------------------------------------+ > >
