Re: [DISCUSSION] Cassandra's code style and source code analysis
Hello everyone, have a great vacation and happy holidays to all!
I've completed a small research about how the classe's import order
rule are spread in the Apache projects. Some of the projects don't
have any restrictions over the imports even if they are using the
checkstyle configuration. The other ones may have only the consensus
over the imports, but they are not reflected in the checkstyle yet
(e.g. Kafka). The conclusion here can only be that there is a very
large variability in the classe's import order, so we have to agree on
the order on our own.
You can find the projects, IDEs and frameworks and their corresponding
classe's import order below:
https://mmuzaf.github.io/blog/Java_Import_Orders.html
Most of the time during development in an IDE the classe's imports
remains collapsed, so from my point of view the following things
related to the classe's import comes into the first place to consider:
- a PR review: newly imports must be clearly visible;
- try to minimize the total amount of affected files;
- the import order rule must be implemented in a simple way and well
supported by IDEs and its plugins;
In addition to the last mentioned option, the checkstyle itself has
some limitations also. For instance, the ImportOrder has a limitation
by design to enforce an empty line between groups ("java", "javax"),
or CustomImportOrder may have only up to 4 custom groups separated by
a blank line.
Based on all of the above I can propose the following classe's order.
All of them are tested on the latest changes from the trunk branch
(commit hash: b171b4ba294126e985d0ee629744516f19c8644e)
1. Total 2 groups, 3072 files to change
```
all other imports
[blank line]
static all other imports
```
2. Total 3 groups, 2345 files to change
```
java.*
javax.*
[blank line]
all other imports
[blank line]
static all other imports
```
3. Total 5 groups, 2968 files to change
```
org.apache.cassandra.*
[blank line]
java.*
[blank line]
javax.*
[blank line]
all other imports
[blank line]
static all other imports
```
4. Total 5 groups, 1792 files to change
```
java.*
javax.*
[blank line]
com.*
net.*
org.*
[blank line]
org.apache.cassandra.*
[blank line]
all other imports
[blank line]
static all other imports
```
5. Total 2 groups, 3114 files to change
```
java.*
javax.*
org.apache.cassandra.*
all other imports
[blank line]
static all other imports
```
Of course, any suggestions are really appreciated.
Please, share your thoughts.
On Thu, 15 Dec 2022 at 17:48, Mick Semb Wever wrote:
>>
>> Another angle I forgot to mention is that this is quite a big patch and
>> there are quite big pieces of work coming, being it CEP-15, for example. So
>> I am trying to figure out if we are ok to just merge this work first and
>> devs doing CEP-15 will need to rework their imports or we merge this after
>> them so we will fix their stuff. I do not know what is more preferable.
>
>
>
> Thank you for bringing this point up Stefan.
>
> I would be actively reaching out to all those engaged with current CEPs,
> asking them the rebase impact this would cause and if they are ok with it.
> The CEPs are our priority, and we have a significant amount of them in
> progress compared to anything we've had for many years.
>
>
>
Re: [DISCUSSION] Cassandra's code style and source code analysis
I like 3 or 4.
We need to be sure we have a way of deactivating the check with code comments
tho, as Java 8 has some bug with import order that can rarely break
compilation, so we need to have some mechanism for permitting a different
import order.
Did we decide any changes to star imports?
> On 22 Dec 2022, at 14:53, Maxim Muzafarov wrote:
>
> Hello everyone, have a great vacation and happy holidays to all!
>
>
> I've completed a small research about how the classe's import order
> rule are spread in the Apache projects. Some of the projects don't
> have any restrictions over the imports even if they are using the
> checkstyle configuration. The other ones may have only the consensus
> over the imports, but they are not reflected in the checkstyle yet
> (e.g. Kafka). The conclusion here can only be that there is a very
> large variability in the classe's import order, so we have to agree on
> the order on our own.
>
> You can find the projects, IDEs and frameworks and their corresponding
> classe's import order below:
> https://mmuzaf.github.io/blog/Java_Import_Orders.html
>
>
> Most of the time during development in an IDE the classe's imports
> remains collapsed, so from my point of view the following things
> related to the classe's import comes into the first place to consider:
>
> - a PR review: newly imports must be clearly visible;
> - try to minimize the total amount of affected files;
> - the import order rule must be implemented in a simple way and well
> supported by IDEs and its plugins;
>
> In addition to the last mentioned option, the checkstyle itself has
> some limitations also. For instance, the ImportOrder has a limitation
> by design to enforce an empty line between groups ("java", "javax"),
> or CustomImportOrder may have only up to 4 custom groups separated by
> a blank line.
>
>
>
> Based on all of the above I can propose the following classe's order.
> All of them are tested on the latest changes from the trunk branch
> (commit hash: b171b4ba294126e985d0ee629744516f19c8644e)
>
>
> 1. Total 2 groups, 3072 files to change
>
> ```
> all other imports
> [blank line]
> static all other imports
> ```
>
> 2. Total 3 groups, 2345 files to change
>
> ```
> java.*
> javax.*
> [blank line]
> all other imports
> [blank line]
> static all other imports
> ```
>
> 3. Total 5 groups, 2968 files to change
>
> ```
> org.apache.cassandra.*
> [blank line]
> java.*
> [blank line]
> javax.*
> [blank line]
> all other imports
> [blank line]
> static all other imports
> ```
>
> 4. Total 5 groups, 1792 files to change
>
> ```
> java.*
> javax.*
> [blank line]
> com.*
> net.*
> org.*
> [blank line]
> org.apache.cassandra.*
> [blank line]
> all other imports
> [blank line]
> static all other imports
> ```
>
> 5. Total 2 groups, 3114 files to change
>
> ```
> java.*
> javax.*
> org.apache.cassandra.*
> all other imports
> [blank line]
> static all other imports
> ```
>
>
> Of course, any suggestions are really appreciated.
> Please, share your thoughts.
>
>> On Thu, 15 Dec 2022 at 17:48, Mick Semb Wever wrote:
>>> Another angle I forgot to mention is that this is quite a big patch and
>>> there are quite big pieces of work coming, being it CEP-15, for example. So
>>> I am trying to figure out if we are ok to just merge this work first and
>>> devs doing CEP-15 will need to rework their imports or we merge this after
>>> them so we will fix their stuff. I do not know what is more preferable.
>>
>>
>>
>> Thank you for bringing this point up Stefan.
>>
>> I would be actively reaching out to all those engaged with current CEPs,
>> asking them the rebase impact this would cause and if they are ok with it.
>> The CEPs are our priority, and we have a significant amount of them in
>> progress compared to anything we've had for many years.
>>
>>
>>
Re: [DISCUSSION] Cassandra's code style and source code analysis
4
I would like to avoid star imports in general. Having them explicitly
enumerated seems to be better, it can not happen we use some import wrongly (by
accident). There is rational behind this here (1).
This check can be configured in such a way that star imports could be possible
on some packages. For example "org.apache.cassandra.tools.NodeTool" has star
import "import org.apache.cassandra.tools.nodetool.*;" where all nodetool
commands are. This can be configured so it would remain to be star import on
that package so we do not need to explicitly import 100+ classes. There might
be more cases like this but they would need to be reviewed more closely.
(1) https://checkstyle.org/config_imports.html#AvoidStarImport
From: Maxim Muzafarov
Sent: Thursday, December 22, 2022 15:52
To: dev@cassandra.apache.org
Subject: Re: [DISCUSSION] Cassandra's code style and source code analysis
NetApp Security WARNING: This is an external email. Do not click links or open
attachments unless you recognize the sender and know the content is safe.
Hello everyone, have a great vacation and happy holidays to all!
I've completed a small research about how the classe's import order
rule are spread in the Apache projects. Some of the projects don't
have any restrictions over the imports even if they are using the
checkstyle configuration. The other ones may have only the consensus
over the imports, but they are not reflected in the checkstyle yet
(e.g. Kafka). The conclusion here can only be that there is a very
large variability in the classe's import order, so we have to agree on
the order on our own.
You can find the projects, IDEs and frameworks and their corresponding
classe's import order below:
https://mmuzaf.github.io/blog/Java_Import_Orders.html
Most of the time during development in an IDE the classe's imports
remains collapsed, so from my point of view the following things
related to the classe's import comes into the first place to consider:
- a PR review: newly imports must be clearly visible;
- try to minimize the total amount of affected files;
- the import order rule must be implemented in a simple way and well
supported by IDEs and its plugins;
In addition to the last mentioned option, the checkstyle itself has
some limitations also. For instance, the ImportOrder has a limitation
by design to enforce an empty line between groups ("java", "javax"),
or CustomImportOrder may have only up to 4 custom groups separated by
a blank line.
Based on all of the above I can propose the following classe's order.
All of them are tested on the latest changes from the trunk branch
(commit hash: b171b4ba294126e985d0ee629744516f19c8644e)
1. Total 2 groups, 3072 files to change
```
all other imports
[blank line]
static all other imports
```
2. Total 3 groups, 2345 files to change
```
java.*
javax.*
[blank line]
all other imports
[blank line]
static all other imports
```
3. Total 5 groups, 2968 files to change
```
org.apache.cassandra.*
[blank line]
java.*
[blank line]
javax.*
[blank line]
all other imports
[blank line]
static all other imports
```
4. Total 5 groups, 1792 files to change
```
java.*
javax.*
[blank line]
com.*
net.*
org.*
[blank line]
org.apache.cassandra.*
[blank line]
all other imports
[blank line]
static all other imports
```
5. Total 2 groups, 3114 files to change
```
java.*
javax.*
org.apache.cassandra.*
all other imports
[blank line]
static all other imports
```
Of course, any suggestions are really appreciated.
Please, share your thoughts.
On Thu, 15 Dec 2022 at 17:48, Mick Semb Wever wrote:
>>
>> Another angle I forgot to mention is that this is quite a big patch and
>> there are quite big pieces of work coming, being it CEP-15, for example. So
>> I am trying to figure out if we are ok to just merge this work first and
>> devs doing CEP-15 will need to rework their imports or we merge this after
>> them so we will fix their stuff. I do not know what is more preferable.
>
>
>
> Thank you for bringing this point up Stefan.
>
> I would be actively reaching out to all those engaged with current CEPs,
> asking them the rebase impact this would cause and if they are ok with it.
> The CEPs are our priority, and we have a significant amount of them in
> progress compared to anything we've had for many years.
>
>
>
Re: [DISCUSSION] Cassandra's code style and source code analysis
I vote for #4. I've always used the convention of having stdlib stuff
first, external stuff second, and same-project imports last. I guess
increasing order of specificity?
Happy Holidays!
Derek
On Thu, Dec 22, 2022 at 7:52 AM Maxim Muzafarov wrote:
>
> Hello everyone, have a great vacation and happy holidays to all!
>
>
> I've completed a small research about how the classe's import order
> rule are spread in the Apache projects. Some of the projects don't
> have any restrictions over the imports even if they are using the
> checkstyle configuration. The other ones may have only the consensus
> over the imports, but they are not reflected in the checkstyle yet
> (e.g. Kafka). The conclusion here can only be that there is a very
> large variability in the classe's import order, so we have to agree on
> the order on our own.
>
> You can find the projects, IDEs and frameworks and their corresponding
> classe's import order below:
> https://mmuzaf.github.io/blog/Java_Import_Orders.html
>
>
> Most of the time during development in an IDE the classe's imports
> remains collapsed, so from my point of view the following things
> related to the classe's import comes into the first place to consider:
>
> - a PR review: newly imports must be clearly visible;
> - try to minimize the total amount of affected files;
> - the import order rule must be implemented in a simple way and well
> supported by IDEs and its plugins;
>
> In addition to the last mentioned option, the checkstyle itself has
> some limitations also. For instance, the ImportOrder has a limitation
> by design to enforce an empty line between groups ("java", "javax"),
> or CustomImportOrder may have only up to 4 custom groups separated by
> a blank line.
>
>
>
> Based on all of the above I can propose the following classe's order.
> All of them are tested on the latest changes from the trunk branch
> (commit hash: b171b4ba294126e985d0ee629744516f19c8644e)
>
>
> 1. Total 2 groups, 3072 files to change
>
> ```
> all other imports
> [blank line]
> static all other imports
> ```
>
> 2. Total 3 groups, 2345 files to change
>
> ```
> java.*
> javax.*
> [blank line]
> all other imports
> [blank line]
> static all other imports
> ```
>
> 3. Total 5 groups, 2968 files to change
>
> ```
> org.apache.cassandra.*
> [blank line]
> java.*
> [blank line]
> javax.*
> [blank line]
> all other imports
> [blank line]
> static all other imports
> ```
>
> 4. Total 5 groups, 1792 files to change
>
> ```
> java.*
> javax.*
> [blank line]
> com.*
> net.*
> org.*
> [blank line]
> org.apache.cassandra.*
> [blank line]
> all other imports
> [blank line]
> static all other imports
> ```
>
> 5. Total 2 groups, 3114 files to change
>
> ```
> java.*
> javax.*
> org.apache.cassandra.*
> all other imports
> [blank line]
> static all other imports
> ```
>
>
> Of course, any suggestions are really appreciated.
> Please, share your thoughts.
>
> On Thu, 15 Dec 2022 at 17:48, Mick Semb Wever wrote:
> >>
> >> Another angle I forgot to mention is that this is quite a big patch and
> >> there are quite big pieces of work coming, being it CEP-15, for example.
> >> So I am trying to figure out if we are ok to just merge this work first
> >> and devs doing CEP-15 will need to rework their imports or we merge this
> >> after them so we will fix their stuff. I do not know what is more
> >> preferable.
> >
> >
> >
> > Thank you for bringing this point up Stefan.
> >
> > I would be actively reaching out to all those engaged with current CEPs,
> > asking them the rebase impact this would cause and if they are ok with it.
> > The CEPs are our priority, and we have a significant amount of them in
> > progress compared to anything we've had for many years.
> >
> >
> >
--
+---+
| 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: [DISCUSSION] Cassandra's code style and source code analysis
Im good with 3 and 4.
> On Dec 22, 2022, at 10:41 AM, Derek Chen-Becker wrote:
>
> I vote for #4. I've always used the convention of having stdlib stuff
> first, external stuff second, and same-project imports last. I guess
> increasing order of specificity?
>
> Happy Holidays!
>
> Derek
>
> On Thu, Dec 22, 2022 at 7:52 AM Maxim Muzafarov wrote:
>>
>> Hello everyone, have a great vacation and happy holidays to all!
>>
>>
>> I've completed a small research about how the classe's import order
>> rule are spread in the Apache projects. Some of the projects don't
>> have any restrictions over the imports even if they are using the
>> checkstyle configuration. The other ones may have only the consensus
>> over the imports, but they are not reflected in the checkstyle yet
>> (e.g. Kafka). The conclusion here can only be that there is a very
>> large variability in the classe's import order, so we have to agree on
>> the order on our own.
>>
>> You can find the projects, IDEs and frameworks and their corresponding
>> classe's import order below:
>> https://mmuzaf.github.io/blog/Java_Import_Orders.html
>>
>>
>> Most of the time during development in an IDE the classe's imports
>> remains collapsed, so from my point of view the following things
>> related to the classe's import comes into the first place to consider:
>>
>> - a PR review: newly imports must be clearly visible;
>> - try to minimize the total amount of affected files;
>> - the import order rule must be implemented in a simple way and well
>> supported by IDEs and its plugins;
>>
>> In addition to the last mentioned option, the checkstyle itself has
>> some limitations also. For instance, the ImportOrder has a limitation
>> by design to enforce an empty line between groups ("java", "javax"),
>> or CustomImportOrder may have only up to 4 custom groups separated by
>> a blank line.
>>
>>
>>
>> Based on all of the above I can propose the following classe's order.
>> All of them are tested on the latest changes from the trunk branch
>> (commit hash: b171b4ba294126e985d0ee629744516f19c8644e)
>>
>>
>> 1. Total 2 groups, 3072 files to change
>>
>> ```
>> all other imports
>> [blank line]
>> static all other imports
>> ```
>>
>> 2. Total 3 groups, 2345 files to change
>>
>> ```
>> java.*
>> javax.*
>> [blank line]
>> all other imports
>> [blank line]
>> static all other imports
>> ```
>>
>> 3. Total 5 groups, 2968 files to change
>>
>> ```
>> org.apache.cassandra.*
>> [blank line]
>> java.*
>> [blank line]
>> javax.*
>> [blank line]
>> all other imports
>> [blank line]
>> static all other imports
>> ```
>>
>> 4. Total 5 groups, 1792 files to change
>>
>> ```
>> java.*
>> javax.*
>> [blank line]
>> com.*
>> net.*
>> org.*
>> [blank line]
>> org.apache.cassandra.*
>> [blank line]
>> all other imports
>> [blank line]
>> static all other imports
>> ```
>>
>> 5. Total 2 groups, 3114 files to change
>>
>> ```
>> java.*
>> javax.*
>> org.apache.cassandra.*
>> all other imports
>> [blank line]
>> static all other imports
>> ```
>>
>>
>> Of course, any suggestions are really appreciated.
>> Please, share your thoughts.
>>
>> On Thu, 15 Dec 2022 at 17:48, Mick Semb Wever wrote:
Another angle I forgot to mention is that this is quite a big patch and
there are quite big pieces of work coming, being it CEP-15, for example.
So I am trying to figure out if we are ok to just merge this work first
and devs doing CEP-15 will need to rework their imports or we merge this
after them so we will fix their stuff. I do not know what is more
preferable.
>>>
>>>
>>>
>>> Thank you for bringing this point up Stefan.
>>>
>>> I would be actively reaching out to all those engaged with current CEPs,
>>> asking them the rebase impact this would cause and if they are ok with it.
>>> The CEPs are our priority, and we have a significant amount of them in
>>> progress compared to anything we've had for many years.
>>>
>>>
>>>
>
>
>
> --
> +---+
> | 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: [DISCUSSION] Cassandra's code style and source code analysis
> > > 3. Total 5 groups, 2968 files to change > > ``` > org.apache.cassandra.* > [blank line] > java.* > [blank line] > javax.* > [blank line] > all other imports > [blank line] > static all other imports > ``` > 3, then 5. There's lots under com.*, net.*, org.* that is essentially the same as "all other imports", what's the reason to separate those? My preference for 3 is simply that imports are by default collapsed, and if I expand them it's the dependencies on other cassandra stuff I'm first grokking. It's also our only imports that lead to cyclic dependencies (which we're not good at).
