+1 avoid star rule On Wed, Dec 7, 2022 at 5:30 PM Maxim Muzafarov <mmu...@apache.org> wrote:
> 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! > >>>> >
