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