Re: Updating our Code Contribution/Style Guide

[bened...@apache.org]

> 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 
Date: Friday, 13 May 2022 at 19:12
To: 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 e

Re: Updating our Code Contribution/Style Guide

[Josh McKenzie]

> 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

Re: Updating our Code Contribution/Style Guide

[Derek Chen-Becker]

On Sat, May 14, 2022 at 8: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.
>

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

Re: Updating our Code Contribution/Style Guide

[Derek Chen-Becker]

On Sat, May 14, 2022 at 11:00 AM Josh McKenzie  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  |
+---+

Re: Updating our Code Contribution/Style Guide

[bened...@apache.org]

> 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 
Date: Saturday, 14 May 2022 at 20:45
To: 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>> 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  |
+-

Re: Updating our Code Contribution/Style Guide

[Derek Chen-Becker]

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 
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 
> *Date: *Saturday, 14 May 2022 at 20:45
> *To: *dev@cassandra.apache.org 
> *Subject: *Re: Updating our Code Contribution/Style Guide
>
>
>
>
>
> On Sat, May 14, 2022 at 8: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.
>
>
>
> 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-Bec