[
https://issues.apache.org/jira/browse/CASSANDRA-17925?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18050348#comment-18050348
]
Michael Semb Wever commented on CASSANDRA-17925:
------------------------------------------------
Shit [~benedict], I didn't realise you had any unaddressed concerns here. It
was my understanding that all concerns were addressed either directly, or
indirectly in their due contexts. The patch has been out for 7 months, and we
took patience between review finished time to merge as well as raising it to
attention on #cassandra-dev too. (Not being defensive, just noting the
surprise.)
On that dev thread you mentioned (at least I read it) that if our existing
rules could be put into checkstyle then they should be. That combined with the
fact that today our checkstyle has always failed CI – we haven't yet introduced
any notion of checkstyle warning vs error. But I am aware that the fact that
`ant check` was part of the CI pipeline has this year surprised, and we were
trying to be extra careful with these changes.
In CASSANDRA-20931 we added a github action that runs `ant check` so that
developers receive ample feedback during dev cycle of checkstyle violations.
And in this ticket we further improved on that concern by ensuring IDEs can fix
import issues (from this ticket AND previous) with a single keyboard shortcut.
I am keen to give folk time to gauge how these things feel in practice. And
then if there's still differing preferences on how this should be done we
tackle it in a new dev thread and ticket. Given introducing the checkstyle
warning level is something new it is IMHO outside of the scope of this ticket
(now that it's reviewed and merged). Same applies to any possible suggestions
of taking `ant check` out of the CI pipeline. Let's keep the discussion open,
but not bound to this ticket please 🙏
I'm also very interested too in the idea of automating import rules with a
linter, instead of introducing checkstyle warnings. As you say we would need
confidence that it was safe (didn't break builds and only touched imports).
Maybe someone has time to look into that during the period we give people the
chance to try out and get familiar with the current improvements ?
> Java source code should have sorted imports as defined in the codestyle
> -----------------------------------------------------------------------
>
> Key: CASSANDRA-17925
> URL: https://issues.apache.org/jira/browse/CASSANDRA-17925
> Project: Apache Cassandra
> Issue Type: Improvement
> Components: Build
> Reporter: Stefan Miklosovic
> Assignee: Maxim Muzafarov
> Priority: Normal
> Labels: code-polishing
> Fix For: 5.1
>
> Attachments: ci_summary_Mmuzaf_cassandra-17925_59.html,
> results_details_Mmuzaf_cassandra-17925_59.tar.xz
>
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> After we cleaned all unused imports in CASSANDRA-17876, there is one more
> task remaining to be done - to add checkstyle for imports order and enforce
> this on build time.
> When the project is imported into IDEA, there is a helper target on Ant
> called "generate-idea-files". ide/idea/codeStyleSettings.xml contains this:
> {code:java}
> <option name="IMPORT_LAYOUT_TABLE">
> <value>
> <package name="java" withSubpackages="true" static="false" />
> <package name="javax" withSubpackages="true" static="false" />
> <emptyLine />
> <package name="com.google.common" withSubpackages="true"
> static="false" />
> <package name="org.apache.log4j" withSubpackages="true"
> static="false" />
> <package name="org.apache.commons" withSubpackages="true"
> static="false" />
> <package name="org.cliffc.high_scale_lib" withSubpackages="true"
> static="false" />
> <package name="org.junit" withSubpackages="true" static="false" />
> <package name="org.slf4j" withSubpackages="true" static="false" />
> <emptyLine />
> <package name="" withSubpackages="true" static="false" />
> <emptyLine />
> <package name="" withSubpackages="true" static="true" />
> </value>
> </option>
> {code}
> This code style is also mentioned in the web page here (minus some details
> which are present in above configuration snippet but not on the web page):
> [https://cassandra.apache.org/_/development/code_style.html] (at the very
> bottom).
> However, when one runs "Optimise imports" in the context menu after
> right-clicking on org.cassandra.apache package, it will refactor the imports
> and it results with hundreds of changes.
> This means that the source code, as-is, does not adhere to the self-imposed
> code style we ship for IDEA.
> If we fix this, we should add checkstyle for it like this:
> {code:java}
> <module name="ImportOrder">
> <property name="groups"
> value="/(^java\.|javax)/,/(com\.google\.common|org\.apache\.log4j|org\.apache\.commons|org\.cliffc\.high_scale_lib|org\.junit|org\.slf4j)/"/>
> <property name="ordered" value="true"/>
> <property name="separated" value="true"/>
> <property name="option" value="bottom"/>
> <property name="separatedStaticGroups" value="false"/>
> <property name="sortStaticImportsAlphabetically" value="true"/>
> </module>
> {code}
> This checkstyle on import order will pass on the source code we run the
> import optimization in the context menu on.
> There is also no enforcement on "all star" imports (org.some.pkg.*).
> Checkstyle has specific module for this:
> [https://checkstyle.org/config_imports.html#AvoidStarImport]
> I propose we should stop to use all-star imports. Same argument holds as
> described there: Rationale: Importing all classes from a package or static
> members from a class leads to tight coupling between packages or classes and
> might lead to problems when a new version of a library introduces name
> clashes.
> This should be applied to test checktyle as well and the source code should
> be refactored on imports too.
> This should be done on cassandra-4.1 as well as for trunk.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
