> Technically it can be two commits which would be merged / pushed at once.
I'll prepare a new pull request containing both of the changes. My previous experience says me that it's really hard to find a reviewer who will be able to go through huge pull requests, that's why initially I've split this into AvoidStarImport and CustomImportOrder rules. So, if you'll help with the review I'm happy to proceed the way you suggested :-) > One thing which needs extra care for ordering imports is that if you order it > in IDEA by right-clicking on a package and choosing organising imports, it > will remove special comments You're right, but this is quite unusual behaviour for me and this seems to be a bug, that hasn't been fixed for a long time [1]. I've tested the same thing for Eclipse and NetBeans and `optimize imports` working there as we expect (no comments removes), so the issue exists only for the IntelliJ IDEA [1]. Despite all of that, we are still on the safe side here - if these comments will be removed by the `optimized import` procedure the build with checkstyle will fail. > I think this is a great time to revisit this ordering. I would say that the imports order is pretty good (probably, except for the blank lines) and the imports order is not as important as it is important that it be the same in all files and automation `optimize imports`. I suggest going through a "minimum change" strategy here. The IntelliJ IDEA has the following configuration with the imports order that most of the classes already fit: import java import javax [blank line] import com.google.common import org.apache.log4j import org.apache.commons import org.cliffc.high_scale_lib import org.junit import org.slf4j [blank line] import all other imports [blank line] import static all other imports We can update the documentation page [2] with this order and implement the same for NetBeans and Eclipse IDE configuration files as well as for the checkstyle config. If everyone is OK with the plan above I'll prepare everything for it. Suggested summary: - use current IntelliJ IDEA imports order as defaults for other IDEs; - update the documentation page; - prepare a single pull request with AvoidStarImport and CustomImportOrder; [1] https://youtrack.jetbrains.com/issue/IDEA-128133/Optimize-Imports-disregards-line-comments [2] https://cassandra.apache.org/_/development/code_style.html On Sun, 11 Dec 2022 at 00:03, Miklosovic, Stefan <stefan.mikloso...@netapp.com> wrote: > > Should the source code obey the AvoidStarImport rule? > > yes > > Should we implement AvoidStarImport and CustomImportOrder in a > single pull request or do it one by one? > > Technically it can be two commits which would be merged / pushed at once. > > One thing which needs extra care for ordering imports is that if you order it > in IDEA by right-clicking on a package and choosing organising imports, it > will remove special comments which are put at the end of the import > statement. We need to be sure you put them back. Look at changes in > CASSANDRA-17055. We need to preserve this. > > Also, we need to be sure that the importing style can be (roughly) set in > each major IDE. (eclipse / netbeans / idea) so if we require some import > style it can be set in IDE like that. I do not know if we have any strong > preference when it comes to this but it definitely does not hurt. > > Also, I see that the current import style is > > java > [blank line] > com.google.common > org.apache.commons > org.junit > org.slf4j > [blank line] > everything else alphabetically > > I think this is a great time to revisit this ordering. I am not particularly > persuaded on this order and why it was choosen. Where has that decision come > from? > > ________________________________________ > From: Maxim Muzafarov <mmu...@apache.org> > Sent: Wednesday, December 7, 2022 18:29 > 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. > > > > > Dear community, > > > I have created the epic with code-style activities to track the progress: > https://issues.apache.org/jira/browse/CASSANDRA-18090 > > In my understanding, there is no need to format whole the code base at > once according to the code style described on the page [1], and the > best strategy here is to go forward with small evolutionary changes. > Thus eventually we will come up with a set of rules convenient for all > members of the community. In my mind, having one commit per an added > code style rule should be easy to look at for a reviewer, the git > commits history as well as rebasing/merging other pull requests that > may be affected by the new rules. > > > I want to raise one more question related to class imports and the > classses import order for a wider discussion. The import order is well > described on the code style page [1], but using wildcard imports is > not mentioned at all. The wildcard imports with their drawbacks has > has already been raised in the JIRA issue [2] and didn't get enough > attention. > > The checkstyle has the rules we are interested in for import control > and they must be considered together. We can implement them in a > single pull request or one by one, or use only the last one: > - AvoidStarImport > - CustomImportOrder > > But still, I think that wildcard imports have more disadvantages > (class names conflicts e.g. java.util.*, java.sql.* or a new version > of a library has name clashes) than advantages and such problems will > be found in later CI cycles. > Currently, I've implemented the AvoidStarImport checkstyle rule in a > dedicated pull request [3][4], so you will be able to see all amount > of the changes with removing wildcard imports. The changes are made > for the checkstyle configuration as well as for code style > configurations for different IDEs we supported. > > So, the open questions here are: > > - Should the source code obey the AvoidStarImport rule [3]? (I think yes); > - Should we implement AvoidStarImport and CustomImportOrder in a > single pull request or do it one by one? > > > Anyway, I will fix the result of the agreement over the > AvoidStarImport rule on the documentation page [1]. > > > > [1] https://cassandra.apache.org/_/development/code_style.html > [2] https://issues.apache.org/jira/browse/CASSANDRA-17925 > [3] https://issues.apache.org/jira/browse/CASSANDRA-18089 > [4] https://github.com/apache/cassandra/pull/2041 > > On Thu, 1 Dec 2022 at 11:55, Claude Warren, Jr via dev > <dev@cassandra.apache.org> wrote: > > > > The last time I worked on a project that tried to implement a coding style > > across the project it was "an education". The short story is that trying > > to "mitigate" the code base, with respect to style, is either a massive > > change or a long slow process. > > > > Arguments here have stated that earlier attempts to have the tooling > > reformat the code did not go well. What we ended up doing was turned on > > the style checker and looked at the number of issues across the project. > > When new code was accepted the number of issues could not rise. Eventually > > most of the code was clean, with a few well coded legacy bits still not up > > to standard. We could do something similar here. Much like code coverage, > > you can't perform a merge unless the number of style errors remains the > > same or decreases. > > > > As with all software rules, this is a strong recommendation as I am certain > > that there are edge/corner case exceptions to be found. > > > > > > > > > > On Wed, Nov 30, 2022 at 3:30 PM Patrick McFadin <pmcfa...@gmail.com> wrote: > >> > >> Why are we still debating build tooling? I think you’re wrong, but I’ve > >> conceded - on the assumption that we can get enough volunteers willing to > >> adopt responsibility for the new world order. > >> > >> Not debating. I am just throwing in my support since I have been in the > >> Camp of Ant. > >> > >> On Wed, Nov 30, 2022 at 1:29 AM Benedict <bened...@apache.org> wrote: > >>> > >>> Why are we still debating build tooling? I think you’re wrong, but I’ve > >>> conceded - on the assumption that we can get enough volunteers willing to > >>> adopt responsibility for the new world order. > >>> > >>> I suggest five long term contributors nominate themselves as the build > >>> file maintainers, and collectively manage a safe and painless migration > >>> for the rest of us - and agree to maintain and develop the new build file > >>> going forwards, and support the community as they adopt it. > >>> > >>> On the topic of over-exuberant linting I will continue to push back. I > >>> think linting our brace rules could make sense since they are atypical, > >>> but more formatting rules than this likely just leads to atrophying > >>> style. Authorship involves thinking about how to present your code; I > >>> don’t want to either encourage lazy authorship or prevent experimentation > >>> with presentation. Both would be bad, and I expect we would struggle to > >>> evolve our style guide again in future as the language evolves. Our brace > >>> rules are a good example everyone unilaterally ignored when lambdas > >>> arrived, as we all recognised they materially harmed the brevity > >>> benefits, and we eventually codified this. > >>> > >>> On migration: auto formatters applied to code that was not written with > >>> the rules in mind will almost unerringly be made a mess of, so having a > >>> tool do this is not acceptable IMO. > >>> > >>> The idea of checkstyle being the source of truth continues to be > >>> untenable and anyone still pushing for this should please engage with my > >>> earlier points on this. > >>> > >>> > >>> On 30 Nov 2022, at 04:06, Patrick McFadin <pmcfa...@gmail.com> wrote: > >>> > >>> > >>> I'm going to +1 what Stefan has said. I've heard on many occasions from > >>> newcomers to the project that having to use Ant is a deterrent. As a > >>> matter of fact, a few weeks ago, I spent a Sunday afternoon helping > >>> somebody trying to build Cassandra and Ant caused a ton of problems. "Ok. > >>> ant really super clean this time" > >>> > >>> Sure it still works for people that have been doing this for years. I > >>> drive a 20 year old Toyota truck, but I'm reminded by my kids often that > >>> it's not cool. So in that spirit, I feel my saying we need to keep Ant is > >>> like saying "You kids get off my lawn!" If it's something that will help > >>> attract new contributors, I'm all for it. > >>> > >>> Patrick > >>> > >>> On Fri, Nov 25, 2022 at 2:22 AM Miklosovic, Stefan > >>> <stefan.mikloso...@netapp.com> wrote: > >>>> > >>>> I agree with what you wrote. How I understand it is that migrating to > >>>> Maven/Gradle makes the project more "attractive" for newcomers. If a > >>>> project is built on "that old un-cool Ant", it might be a little bit > >>>> off-putting and questionable if we are "stuck in the past on build > >>>> systems and not progressing". > >>>> > >>>> So in that sense I agree this is more "marketing" rather than > >>>> technological question but on the other hand, does not Maven/Gradle > >>>> allow us to modularize the project better? Maybe we would like to > >>>> modularize but nobody is up to that because build system makes it > >>>> impossible or at least quite inconvenient to do so. Do you really think > >>>> there are not any significant benefits to switch even if it "just works" > >>>> now? > >>>> > >>>> ________________________________________ > >>>> From: Benedict <bened...@apache.org> > >>>> Sent: Friday, November 25, 2022 11:07 > >>>> 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. > >>>> > >>>> > >>>> > >>>> > >>>> There’s always a handful of people asking for it, but notably few if any > >>>> of the full time contributors doing the majority of the core development > >>>> of Cassandra. It strikes me as something very appealing to others, but > >>>> less so to those wanting to get on with development. > >>>> > >>>> I never really see a good argument articulated for the migration, > >>>> besides general hand waving that ant is old, and people like newer build > >>>> systems. Ant is working fine, so there isn’t a strong technical reason > >>>> to replace it, and there are good organisational reasons not to. > >>>> > >>>> Why do you consider a migration inevitable? > >>>> > >>>> > >>>> > >>>> > On 25 Nov 2022, at 09:58, Miklosovic, Stefan > >>>> > <stefan.mikloso...@netapp.com> wrote: > >>>> > > >>>> > Interesting take on Ant / no-Ant, Benedict. I am very curious how > >>>> > this unfolds. My long-term perception is that changing it to something > >>>> > else is more or less inevitable but if there is a broader consensus to > >>>> > not do that .... well. > >>>> > > >>>> > ________________________________________ > >>>> > From: Benedict <bened...@apache.org> > >>>> > Sent: Friday, November 25, 2022 10: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. > >>>> > > >>>> > > >>>> > > >>>> > > >>>> > I was in a bit of a rush last night. I should say that I’m of course > >>>> > +1 a general endeavour to clean this up, and to expand our use of > >>>> > linters, and I appreciate your volunteering to help out in this way > >>>> > Maxim. > >>>> > > >>>> > However, responding to Stefan, I’m pretty -1 migrating from ant to > >>>> > another build system without really good reason. Migration has a real > >>>> > cost to productivity for all existing contributors, and the phantom of > >>>> > increasing new contributions has never paid off historically. I’m all > >>>> > for easing people into participation, but not at penalty to the > >>>> > existing contributor base. > >>>> > > >>>> > If the only reason is to make it easier to open in a different IDE, we > >>>> > can perhaps have some basic build files outlining code structure for > >>>> > importing, that are compatible with our canonical ant build? We could > >>>> > perhaps even generate them. > >>>> > > >>>> > > >>>> >> On 25 Nov 2022, at 09:35, Miklosovic, Stefan > >>>> >> <stefan.mikloso...@netapp.com> wrote: > >>>> >> > >>>> >> For the record, I was testing that same combo Claude mentioned and > >>>> >> it did not work out of the box but it is definitely possible to set > >>>> >> up successfully. I do not remember the details. > >>>> >> > >>>> >> To replay to Maxim, it all seems good to me, roughly, but I humbly > >>>> >> think it all boils down to Maven/Gradle refactoring and on top of > >>>> >> that we can do all else. > >>>> >> > >>>> >> For example, there is (1) where the solution, besides fixing the > >>>> >> tests, is to introduce an Ant task which would check this on build. > >>>> >> That being said, how is that going to look like when we change Ant > >>>> >> for something else? That stuff suddenly becomes obsolete. > >>>> >> > >>>> >> This case maybe applies to other problems we want to solve as well. I > >>>> >> do not want to do something tailored for one build system just to > >>>> >> rewrite it all or to spend significant amount of time on that again > >>>> >> when we switch the build system. > >>>> >> > >>>> >> For that reason I think changing Ant for something else should be top > >>>> >> priority (as I understand that it the hot topic for community for > >>>> >> very long time) and then everything else should follow. We should > >>>> >> spend time on things mentioned only in case they do not collide with > >>>> >> any build system at all. > >>>> >> > >>>> >> (1) https://issues.apache.org/jira/browse/CASSANDRA-17964 > >>>> >> > >>>> >> Stefan > >>>> >> > >>>> >> ________________________________________ > >>>> >> From: Claude Warren, Jr via dev <dev@cassandra.apache.org> > >>>> >> Sent: Friday, November 25, 2022 10:16 > >>>> >> 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. > >>>> >> > >>>> >> > >>>> >> > >>>> >> +1 for the concept as a whole. I am certain I could find nits to > >>>> >> pick if I looked deeply. > >>>> >> > >>>> >> @mck -- I did have a problem with Cassandra + Eclipse + Java11 > >>>> >> (Classpath). I gave up and am spending time trying to learn > >>>> >> IntelliJ. I also mentioned it in one of the discussion areas. > >>>> >> > >>>> >> Claude > >>>> >> > >>>> >> On Thu, Nov 24, 2022 at 8:55 PM Mick Semb Wever > >>>> >> <m...@apache.org<mailto:m...@apache.org>> wrote: > >>>> >> Thank you for a solid write up Maxim. And welcome to Cassandra, it's > >>>> >> very positive to see you here. > >>>> >> > >>>> >> I whole-heartedly agree with nearly everything you write. Some input > >>>> >> and questions inline. > >>>> >> > >>>> >> > >>>> >>> > >>>> >>> As you may know, the infrastructure team has disabled public sign-up > >>>> >>> to ASF JIRA (the GitHub issues are recommended instead). > >>>> >>> > >>>> >> > >>>> >> > >>>> >> I suspect (based on chatter in general, but not on dev@ AFAIK) is to > >>>> >> avoid GH issues and stick with jira. The sign-up hurdle we will > >>>> >> document on the website: CASSANDRA-18064 > >>>> >> > >>>> >> > >>>> >> > >>>> >>> == 1. Make the checkstyle config a single point of truth for the > >>>> >>> source code style. == > >>>> >>> > >>>> >>> The checkstyle is already used and included in the Cassandra project > >>>> >>> build lifecycle (ant command line, Jenkins, CircleCI). There is no > >>>> >>> need to maintain code style configurations for different types of > >>>> >>> IDEs > >>>> >>> (e.g. IntelliJ inspections configuration) since the checkstyle.xml > >>>> >>> file can be directly imported to IDE used by a developer. This is > >>>> >>> fair > >>>> >>> for Intellij Idea, NetBeans, and Eclipse. > >>>> >> > >>>> >> > >>>> >> Big +1 > >>>> >> > >>>> >> > >>>> >>> So, I propose to focus on the checks themselves and checking pull > >>>> >>> requests with automation scripts, rather than maintaining these > >>>> >>> integrations. The benefits here are avoiding all issues with > >>>> >>> maintaining configurations for different IDEs. Another good advantage > >>>> >>> of this approach would be the ability to add new checkstyle rules > >>>> >>> without touching IDE configuration - and such tickets will be LFH and > >>>> >>> easy to commit. > >>>> >>> > >>>> >>> The actions points here are: > >>>> >>> > >>>> >>> - create an umbrella JIRA ticket for all checkstyle issues e.g. [8] > >>>> >>> (or label checkstyle); > >>>> >>> - add checkstyle to GitHub pull requests using GitHub actions > >>>> >>> (execute > >>>> >>> ant command); > >>>> >> > >>>> >> > >>>> >> Instead of custom GHA scripting, please use our existing > >>>> >> cassandra-artifact.sh (which should already include all such checks). > >>>> >> > >>>> >> Something like > >>>> >> https://github.com/apache/cassandra/compare/cassandra-3.11...thelastpickle:cassandra:mck/github-actions/3.11 > >>>> >> > >>>> >> > >>>> >> > >>>> >>> == 3. Enable pushing backwards build results for both Jenkins and > >>>> >>> CircleCI to GitHub pull requests. == > >>>> >>> > >>>> >>> The goal here is to have a "green checkbox" for those GitHub pull > >>>> >>> requests that have corresponding Jenkins/CircleCI runs. According to > >>>> >>> the following links, it is completely possible to have those. > >>>> >>> > >>>> >>> https://github.com/jenkinsci/github-branch-source-plugin > >>>> >>> https://circleci.com/docs/enable-checks/ > >>>> >>> > >>>> >>> Moreover, the GitHub Branch Source Plugin is already enabled for the > >>>> >>> Cassandra project [16]. The same seems should work the same way for > >>>> >>> CirleCI, but I have faced the infrastructure team comment [17] that > >>>> >>> describes admin permissions are required for that, so the question is > >>>> >>> still open here. I could dig a bit more once we've agreed on it. > >>>> >>> > >>>> >>> The actions points here are: > >>>> >>> - enable Jenkins integration for GitHub pull requests; > >>>> >>> - enable CircleCI integration for GitHub pull requests; > >>>> >> > >>>> >> > >>>> >> Some folk use CircleCI, some use ci-cassandra. The green checkbox idea > >>>> >> is great, so long as it's optional. We don't want PRs triggering the > >>>> >> runs either (there are other mechanisms for triggering for now). > >>>> >> > >>>> >> > >>>> >>> The actions points here are: > >>>> >>> > >>>> >>> - initiate a wide survey for user and dev lists, to get to know about > >>>> >>> the usages; > >>>> >>> - remove those configurations that are not used anymore; > >>>> >>> - force migration from Ant to Gradle/Maven; > >>>> >> > >>>> >> > >>>> >> Let's leave this out for now. There's too many unknowns here. If > >>>> >> there's an IDE configuration that's broken, no one has reported it for > >>>> >> ages, and no one is around to fix it, then I say we should raise the > >>>> >> discussion to remove it. > >>>> >> > >>>> >> The Gradle/Maven migration is a hot one, contribute to that discussion > >>>> >> but let's not tangle this work up with it, IMHO. > >>>> >> > >>>> >> Totally agree that IDE project files should be as light weight as > >>>> >> possible. > >>>> >> > >>>> >> > >>>> >>> Summarizing everything proposed above I think it is possible to > >>>> >>> simplify adding small contributions easier to the codebase as well as > >>>> >>> save a bunch of committer's time. > >>>> >>> > >>>> >>> So, > >>>> >>> WDYT about the things described above? > >>>> >>> Should I create a CEP for this? > >>>> >> > >>>> >> > >>>> >> I see no need for a CEP here. An epic and tickets will work. > >>>> >> Again, thanks for the input Maxim! > >>>>
