> 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<mailto:bened...@apache.org> <bened...@apache.org<mailto: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 | +---------------------------------------------------------------+
