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<mailto:de...@chen-becker.org>> wrote: Looks great! On Mon, May 30, 2022, 5:37 AM bened...@apache.org<mailto:bened...@apache.org> <bened...@apache.org<mailto:bened...@apache.org>> wrote: Any more feedback around this? Everyone happy with the latest proposal? From: bened...@apache.org<mailto:bened...@apache.org> <bened...@apache.org<mailto:bened...@apache.org>> Date: Sunday, 15 May 2022 at 15:08 To: dev@cassandra.apache.org<mailto:dev@cassandra.apache.org> <dev@cassandra.apache.org<mailto: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<mailto:de...@chen-becker.org>> Date: Saturday, 14 May 2022 at 20:56 To: dev@cassandra.apache.org<mailto:dev@cassandra.apache.org> <dev@cassandra.apache.org<mailto: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<mailto: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 | +---------------------------------------------------------------+
