+1 on locking on one codestyle and I think that is what current checkstyle rules serving.
For automatic applying part, we suggest developing by IDEA and with Checkstyle Plugin on IDEA applying checkstyle suggestion is also automatic. One short coming for spotless is that we can hardly adjust rules if the project has its own issues to overcome. IIRC only several pre-defined rules and a clumsy general regex substitution rule can be used. FYI my team started with spotless but ended up with checkstyle with few rules and Checkstyle Plugin for automation. Codestyle, in another perspective, is part of cognition of developers working in project, not something we just apply before pull request. No matter how much automation introduced, most of developers will converge working with the configured codestyle. Best, tison. Kostas Kloudas <kklou...@gmail.com> 于2020年10月7日周三 下午6:37写道： > Hi all, > > +1 for enforcing "a" codestyle using "a" tool. > > As the project grows both in terms of LOCs and contributors, this > becomes more and more important as it eliminates some potential points > of friction without any additional effort. > > From the discussion, I am leaning towards having a single commit with > all the codestyle-related changes. This will avoid sprinkling > refactoring commits all over the place for the next year or more. But > if this is the price to pay for having consensus on a tool, then I am > ok with gradual implementation. I believe that the value added by > having an automated process of enforcing a codestyle exceeds the cost > of the nuisance of gradual refactoring. > > As for the actual format, I like the google-java-format but again, if > the community agrees on a different one I would not oppose that (as > long as it does not use the same amount of indentation for method args > and method body :P). > > Cheers, > Kostas > > On Wed, Oct 7, 2020 at 10:26 AM Chesnay Schepler <ches...@apache.org> > wrote: > > > > To me, ratchet seems to combine the worst aspects of both approaches. > > You get disruptive changes, but only in singular files, > > for something mundane as a typo fix or import change, which would be > > annoying to keep separate from the actual functional changes in a PR. > > > > On 10/7/2020 10:04 AM, Matthias Pohl wrote: > > > I find the ratchet feature you're suggesting interesting. But Arvid > has a > > > point referring to the blog post about ignoring revisions in git blame > [1]. > > > Adding the configuration file for commits to ignore revs as proposed > in the > > > blog post makes it even easier. One problem I see is that this is not > > > supported by Github (yet?) [2] as mentioned in [1]. > > > > > > Considering all that I prefer applying the code style in one go. I > have no > > > strong opinion on what codestyle is the best. > > > > > > PS: We used spotless in the project I previously worked on. It was > > > convenient to use. > > > > > > [1] > > > > https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame > > > [2] > > > > https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256 > > > > > > On Tue, Oct 6, 2020 at 6:00 PM Aljoscha Krettek <aljos...@apache.org> > wrote: > > > > > >> Maybe I wasn't very clear on how the ratchet works. The changes are > > >> gradual yes, but it doesn't leave you an option: if you touch a file > you > > >> will it will have to conform to the coding style afterwards. It's not > > >> like the previous gradual process we had before where it would be > based > > >> on people actively working towards a style. > > >> > > >> That being said, I also completely like the option of just doing one > big > > >> change commit. > > >> > > >> Regarding actual coding styles: we're a bit limited by what tools > exist. > > >> I like Spotless because it can be used to both check and apply a > style. > > >> Then you need a formatter that works with Spotless and of those we > only > > >> have the Eclipse Formatter, google-java-format, and Prettier. Prettier > > >> is a Javascript tool that I would like to avoid. Eclipse is doable but > > >> you need to fiddle with configuration files. I like google-java-format > > >> because of it's take-it-or-leave-it approach. You either use the style > > >> or you don't but it's very thorough. The downside is that it won't do > > >> tabs-only formatting. > > >> > > >> Best, > > >> Aljoscha > > >> > > >> On 06.10.20 17:43, Arvid Heise wrote: > > >>> After having written that I did a quick search, you can even use git > > >> blame > > >>> with one big massive change commit [1], which would further help the > idea > > >>> of "just get over with it". > > >>> > > >>> With that option, I'd even change all whitespaces if the community > thinks > > >>> that it's a better option (a separate discussion that I'll gladly > skip). > > >>> > > >>> [1] > > >>> > > >> > https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame > > >>> On Tue, Oct 6, 2020 at 5:38 PM Arvid Heise <ar...@ververica.com> > wrote: > > >>> > > >>>> I'm also +1 for automatically enforceable code style. > > >>>> > > >>>> I also would just go over it as Chesnay said. While it makes some > > >> changes > > >>>> a bit harder to track (inline git blame), it's easy to skip over in > any > > >> git > > >>>> history and if it's only one massive commit, then it's also much > easier > > >> to > > >>>> ignore than many gradual changes. Further, if we just do it once, > git > > >> blame > > >>>> will quickly become more reliable again. > > >>>> > > >>>> Btw I completely don't care about the code style as long as it plays > > >> well > > >>>> with IntelliJ (it used to be different, but things change :p). > > >>>> > > >>>> On Tue, Oct 6, 2020 at 5:23 PM Chesnay Schepler <ches...@apache.org > > > > >>>> wrote: > > >>>> > > >>>>> We shouldn't switch to spaces _now_; cutting this bit from your > > >> proposal > > >>>>> will massively simplify things and there's hardly any value in > changing > > >>>>> it. > > >>>>> > > >>>>> Also I'm getting rather tired of this constant idea of "gradual > > >>>>> application". We've been doing this for 2-3 years now since we > > >>>>> introduced Checkstyle and basically got nowhere. We should just > bite > > >> the > > >>>>> bullet and get it over with; we could've solved this whole problem > > >>>>> already. > > >>>>> > > >>>>> In conclusion, I'm +1 on finally locking down the codestyle and > > >> applying > > >>>>> it immediately, I'm -1 on any gradual application scheme because > they > > >>>>> _just don't work_. > > >>>>> > > >>>>> On 10/6/2020 2:15 PM, Aljoscha Krettek wrote: > > >>>>>> Hi All, > > >>>>>> > > >>>>>> I know I know, but please keep reading because I recently learned > > >>>>>> about some new developments in the area of coding-style > automation. > > >>>>>> > > >>>>>> The tool I would propose we use is Spotless > > >>>>>> (https://github.com/diffplug/spotless). This doesn't come with a > > >>>>>> formatter but allows using other popular formatters such as > > >>>>>> google-java-format. The nice thing about Spotless is that it > serves as > > >>>>>> a verifier for CI but can also apply the configured style > > >>>>>> automatically. That is, for the programmer all she has to do is > `mvn > > >>>>>> spotless:apply` to fix any style violations. > > >>>>>> > > >>>>>> An interesting feature, which was (somewhat) recently added is > > >>>>>> "ratchet" > > >>>>>> ( > > >> > https://github.com/diffplug/spotless/blob/main/plugin-maven/README.md#ratchet > > >> ). > > >>>>>> With this, you can set up Spotless to only apply it's rules to > files > > >>>>>> that were changed after a configured commit. This would allow a > > >>>>>> gradual application of the new coding style instead of one big > change. > > >>>>>> > > >>>>>> If we decide to use Spotless, we would of course also have to > decide > > >>>>>> on a coding style. For this I would propose google-java-format, > which > > >>>>>> the flink-statefun project uses. The main difference from our > current > > >>>>>> "style" is that this uses spaces instead of tabs for indentation. > By > > >>>>>> default it would be 2 spaces but it can be configured to use 4 > spaces > > >>>>>> which would make code look more or less like our current style. > There > > >>>>>> are no more configuration knobs, so using tabs is not an option. > > >>>>>> > > >>>>>> Finally, why should we do this? I think most engineers agree that > > >>>>>> having a common enforced style is good to have so I only want to > > >>>>>> highlight a few thoughts here about things we could improve: > > >>>>>> > > >>>>>> - No more nits about coding style in reviews, this makes it > easier > > >>>>>> for both the reviewer and developer > > >>>>>> > > >>>>>> - No manual fixing of Checkstyle errors because Spotless can > do that > > >>>>>> automatically > > >>>>>> > > >>>>>> - Because Flink is such a big project little islands of coding > style > > >>>>>> have formed between people that commonly work on components. It > can be > > >>>>>> a nuisance when you work on a different component and then > reviewers > > >>>>>> don't like your typical coding style. And you first have to get > used > > >>>>>> to the slight differences in style when reading code. > > >>>>>> > > >>>>>> There are also downsides I see in this: > > >>>>>> > > >>>>>> - We break the history, but both "git blame" and modern > IntelliJ can > > >>>>>> ignore whitespace when attributing changes. So for files that are > > >>>>>> already "well" formatted not much would change. > > >>>>>> > > >>>>>> - In the short-term it will be harder to apply changes both to > > >> master > > >>>>>> and one of the release-x branches because formatting will be > > >>>>>> different. I think this is not too hard though because Spotless > can > > >>>>>> automatically apply the style. > > >>>>>> > > >>>>>> In summary, we would have some short-term pain with this but I > think > > >>>>>> it would be good in the long run. What are your thoughts? > > >>>>>> > > >>>>>> Best, > > >>>>>> Aljoscha > > >>>>>> > > >>>>> > > >>>> -- > > >>>> > > >>>> Arvid Heise | Senior Java Developer > > >>>> > > >>>> <https://www.ververica.com/> > > >>>> > > >>>> Follow us @VervericaData > > >>>> > > >>>> -- > > >>>> > > >>>> Join Flink Forward <https://flink-forward.org/> - The Apache Flink > > >>>> Conference > > >>>> > > >>>> Stream Processing | Event Driven | Real Time > > >>>> > > >>>> -- > > >>>> > > >>>> Ververica GmbH | Invalidenstrasse 115, 10115 Berlin, Germany > > >>>> > > >>>> -- > > >>>> Ververica GmbH > > >>>> Registered at Amtsgericht Charlottenburg: HRB 158244 B > > >>>> Managing Directors: Timothy Alexander Steinert, Yip Park Tung > Jason, Ji > > >>>> (Toni) Cheng > > >>>> > > >>> > > >
