Fair enough. Thanks for the perspective, and the doc is shaping up nicely! On Sat, May 14, 2022 at 2:28 PM bened...@apache.org <bened...@apache.org> wrote:
> > having the policy be enums by default as opposed to just recommending > them > > > > This might be a stylistic issue. “Prefer an enum to Boolean properties” is > imperative voice, and is meant to be read as “you should use enums, not > booleans, unless you have overriding reasons not to” – perhaps the example > scenarios that follow, in which they are most strongly indicated, weaken > the effect. > > > > I’m sure I can tweak the language, but overall I have tried to avoid > making anything an explicit dictat in this style guide. It’s somewhere > between a policy and a set of recommendations, as I think it is preferable > to leave the author and reviewer to make final determinations, and also to > avoid imbuing documents like this with too much power (and making them too > contentious). > > > > I’ll see about tweaking it along with your other suggestions. > > > > *From: *Derek Chen-Becker <de...@chen-becker.org> > *Date: *Saturday, 14 May 2022 at 20:45 > *To: *dev@cassandra.apache.org <dev@cassandra.apache.org> > *Subject: *Re: Updating our Code Contribution/Style Guide > > > > > > On Sat, May 14, 2022 at 8:24 AM bened...@apache.org <bened...@apache.org> > wrote: > > > I'm in favor of codifying the usage of @NotNull and @Nullable > stylistically. +1 > > > > I’m in favour of the use of _*one*_ of @Nullable and @NotNull, preferably > the former since we already use it and it’s more reasonable to have a > default of non-null variables, parameters and properties. > > > > However, I’m not confident in how to craft guidance for these annotations. > I don’t think they should be used in every place a variable or property > might be null, only in places where it is surprising or otherwise > informative to a reader that they might be null. Annotating every property > and variable with @NonNull or @Nullable would seriously pollute the screen, > and probably harm legibility more than help. > > > > At the very least we should mention @Nullable and invite authors to use it > where it aids clarity, but if somebody has a good proposal for better > guidance I’m all ears. > > > > Yes, unfortunately there's a whole menagerie of these types of > annotations, and I didn't mean both. If we're already using Nullable (from > Findbugs) that's the better one anyway because you can specify the when > parameter. It's also supported by languages like Kotlin for nullable types > if we were ever considering a language that wouldn't require polluting the > screen for a bit more safety ;) > > > > Overall I think that an assumption that all variables are null unless > explicitly marked is probably a reasonable first step if it's not already > in place, but it's also a good intention more than a mechanism and I'll put > some thought into other ways we can improve the situation without impacting > legibility. > > > > > > > > > I think extra clarity and social pressure around "Never catch Exception > or Throwable unless you explicitly rethrow them" sounds valuable > > > > We already stipulate that you should always rethrow exceptions, but this > is very vague. I will try to tidy this up. On the whole, though, we have a > fail-fast approach to processing commands, so we mostly just propagate, > with exception handlers existing only for clean-up purposes (except in > particular circumstances, usually involving checked exceptions like > InterruptedException). So we mostly *do* catch Throwable (and rethrow), I > think, which is what informed the current vague formulation. > > > > Sure, rethrow after cleanup seems reasonable, but I think that should be > the explicit exception rather than an assumption of our approach to error > handling. > > > > > I would recommend that we strengthen the recommendation for using enums > for Boolean properties for any type that is used in method parameters > > > > I’m unsure about this. I am not against it per se, but the more enums we > have the more clashes of enum identifiers we have, and this can cause > confusion particularly with static imports, and in some cases the Boolean > property will have a very obvious effect. I prefer to leave some decisions > to the author, since we have expressed a strong preference here for the > author to consider. But perhaps a blanket policy would do more good than > harm. I could endorse it, and am relatively neutral. > > > > > > To be clear, I think there should always be room for (clearly documented) > exceptions, so I was thinking more of having the policy be enums by default > as opposed to just recommending them. I've been thinking that as part of > the guidelines it might be good to have some examples of both (here's how > you can use an enum, but here's a case where a boolean was simple and > clear), so let me dig around and see if I can find some code to point to. > > > > 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 | > > +---------------------------------------------------------------+ > > > -- +---------------------------------------------------------------+ | 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 | +---------------------------------------------------------------+
