> At the very least we should mention @Nullable and invite authors to use it > where it aids clarity, Thanks for taking the time to better articulate the appropriate constraints on this Benedict - I think "always use these annotations" would have very poor signal / noise ratios, but in cases where something's nullability has semantic implications (in a state machine or conditional checks in the logic for instance), I think the annotations can be super helpful. For people that aren't the original author, to be clear.
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". If,* at authors discretion*, we encourage the usage of both we effectively have three states: @Nullable, @NotNull, or... not annotated so don't make any assumptions and/or we don't think it's relevant. 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. :) On Sat, May 14, 2022, at 10:24 AM, 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. > > > 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. > > > When we reference the "Sun Java coding conventions" can we have a canonical > > link > > Yes, this actually is a modification to our existing style guide here: > https://cassandra.apache.org/_/development/code_style.html which has such a > link, it was just lost in the copy/paste to the Google Doc, but I will > restore on commit. > > > The guidance on brace placement seems to contradict the Java coding > > conventions > > Yep, this is a legacy we all probably wish we didn’t inherit, but we did and > it is too late to change it. > > > The doc doesn't seem to cover a recommendation for braces with single-line > > bodies of conditional/loop statements > > Actually the guide explicitly permits this, and this is for reasons of the > brace rule above. Single statement if/for/while loops can rapidly pollute a > method with the additional spacing. The legibility benefits of permitting > this elision far outweigh any potential protection from semi-colon mistakes. > > > I like the section on Method clarity, but I would also call out non-trivial > > predicate logic as a candidate for encapsulation in its own method > > > I think it’s technically captured under “computing an intermediate result” > but you’re right that we could expand this to expressly include complex > predicates – particularly those that can be given a descriptive name. > > > 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. > > > *From: *Josh McKenzie <jmcken...@apache.org> > *Date: *Friday, 13 May 2022 at 19:12 > *To: *dev@cassandra.apache.org <dev@cassandra.apache.org> > *Subject: *Re: Updating our Code Contribution/Style Guide >> Should we consider @NotNull/@Nullable or other annotations besides @Override? > I'm in favor of codifying the usage of @NotNull and @Nullable stylistically. > +1 >> >> In the exception handling section should we discuss using the most >> applicable exception type for the handler? I.e. don't catch Exception or >> Throwable? This probably falls under the don't silently swallow or log >> exceptions paragraph > We have been bitten enough times by swallowing exceptions that I think extra > clarity and social pressure around "Never catch Exception or Throwable unless > you explicitly rethrow them" sounds valuable. +1 to this as well. > > > On Fri, May 13, 2022, at 1:24 PM, Chen-Becker, Derek wrote: >> I have a couple of questions/comments (in no particular order): >> >> >> >> * When we reference the "Sun Java coding conventions" can we have a >> canonical link to that so that people don't have to make an assumption or >> try and find the version we're talking about? Are we referring to the (now >> Oracle) version here, or something else? >> https://www.oracle.com/java/technologies/javase/codeconventions-contents.html >> * I would recommend that we strengthen the recommendation for using enums >> for Boolean properties for any type that is used in method parameters. In my >> experience the improvement in readability at the call site outweighs the >> (modest, IMHO) cost of introducing a new enum, and the enum also provides a >> useful "handle" for providing documentation on the semantics of the flag. >> There are already a lot of Boolean parameters in use in the codebase and I >> can take a look at what it would take to clean these up >> * I like the section on Method clarity, but I would also call out >> non-trivial predicate logic as a candidate for encapsulation in its own >> method >> * Should we consider @NotNull/@Nullable or other annotations besides >> @Override? >> * In the exception handling section should we discuss using the most >> applicable exception type for the handler? I.e. don't catch Exception or >> Throwable? This probably falls under the don't silently swallow or log >> exceptions paragraph >> * The guidance on brace placement seems to contradict the Java coding >> conventions if we place the opening brace on a new line. Is that intentional >> or am I misreading the statement? Would it be clearer to link to a specific >> style as defined somewhere (e.g. >> https://en.wikipedia.org/wiki/Indentation_style#Variant:_Java) >> * The doc doesn't seem to cover a recommendation for braces with >> single-line bodies of conditional/loop statements. In my own experience it >> makes it easier to read if we uniformly used braces everywhere, but it does >> look like there are quite a few places in the code where we have unbraced ifs >> >> >> Overall the doc is well written and carefully considered, and I appreciate >> all of the work that went into it! >> >> >> >> Cheers, >> >> >> >> Derek >> >> >> >> *From: *"bened...@apache.org" <bened...@apache.org> >> *Reply-To: *"dev@cassandra.apache.org" <dev@cassandra.apache.org> >> *Date: *Friday, May 13, 2022 at 6:41 AM >> *To: *"dev@cassandra.apache.org" <dev@cassandra.apache.org> >> *Subject: *RE: [EXTERNAL]Updating our Code Contribution/Style Guide >> >> >> >> *CAUTION*: This email originated from outside of the organization. Do not >> click links or open attachments unless you can confirm the sender and know >> the content is safe. >> >> >> >> It’s been a couple of months since I opened this discussion. I think I have >> integrated the feedback into the google doc. Are there any elements anyone >> wants to continue discussing, or things I have not fully addressed? I’ll >> take an absence of response as lazy consensus to commit the changes to the >> wiki. >> >> >> >> >> >> >> >> *From: *bened...@apache.org <bened...@apache.org> >> *Date: *Monday, 14 March 2022 at 09:41 >> *To: *dev@cassandra.apache.org <dev@cassandra.apache.org> >> *Subject: *Updating our Code Contribution/Style Guide >> >> Our style guide hasn’t been updated in about a decade, and I think it is >> overdue some improvements that address some shortcomings as well as modern >> facilities such as streams and lambdas. >> >> >> >> Most of this was put together for an effort Dinesh started a few years ago, >> but has languished since, in part because the project has always seemed to >> have other priorities. I figure there’s never a good time to raise a >> contended topic, so here is my suggested update to contributor guidelines: >> >> >> >> https://docs.google.com/document/d/1sjw0crb0clQin2tMgZLt_ob4hYfLJYaU4lRX722htTo >> >> >> >> Many of these suggestions codify norms already widely employed, sometimes in >> spite of the style guide, but some likely remain contentious. Some >> potentially contentious things to draw your attention to: >> >> >> >> * Deemphasis of getX() nomenclature, in favour of richer set of prefixes >> and more succinct simple x() to retrieve where clear >> * Avoid implementing methods, incl. equals(), hashCode() and toString(), >> unless actually used >> * Modified new-line rules for multi-line function calls >> * External dependency rules (require DISCUSS thread before introducing) >> >> >> >> >
