+1 it works really well in StateFun for quite some time. On Thursday, December 17, 2020, Wei Zhong <weizhong0...@gmail.com> wrote:
> +1 for the coding style automation. Thanks for driving this! > > Best, > Wei > > > 在 2020年12月17日，10:10，Xingbo Huang <hxbks...@gmail.com> 写道： > > > > +1 asap. Spotless can greatly help us save the time of fixing checkstyle > > errors. > > > > Best, > > Xingbo > > > > Kostas Kloudas <kklou...@gmail.com> 于2020年12月17日周四 上午4:14写道： > > > >> +1 asap from my side as well. > >> > >> On Wed, Dec 16, 2020 at 8:04 PM Arvid Heise <ar...@ververica.com> > wrote: > >>> > >>> +1 asap. > >>> > >>> On Wed, Dec 16, 2020 at 7:44 PM Robert Metzger <rmetz...@apache.org> > >> wrote: > >>> > >>>> +1 > >>>> > >>>> Thanks for driving this. > >>>> > >>>> On Wed, Dec 16, 2020 at 7:33 PM Chesnay Schepler <ches...@apache.org> > >>>> wrote: > >>>> > >>>>> +1 to set this up ASAP. Now's a good time to (finally) finalize this > >>>>> effort with a new release cycle having started and > >> christmas/vacations > >>>>> being around the corner. > >>>>> > >>>>> On 12/16/2020 7:20 PM, Aljoscha Krettek wrote: > >>>>>> Let's try and conclude this discussion! I've prepared a PoC that > >> uses > >>>>>> Spotless with google-java-format to do the formatting: > >>>>>> https://github.com/aljoscha/flink/commits/flink-xxx-spotless > >>>>>> > >>>>>> To summarize from earlier discussion, the main benefits are: > >>>>>> - no more worrying about code style, both as reviewer and > >> implementer > >>>>>> - everyone can configure their IDE to auto-format, there will > >> never > >>>>>> be unrelated formatting changes > >>>>>> > >>>>>> Also, this uses git's blame.ignoreRevsFile to add a file that tells > >>>>>> git blame/IntelliJ to ignore the refactoring commit. However, you > >> need > >>>>>> to manually configure your git for that using: > >>>>>> > >>>>>> $ git config blame.ignoreRevsFile .git-blame-ignore-revs > >>>>>> > >>>>>> You can check out the PoC, look at the code in an IDE, play around > >>>>>> with blame/annotations. > >>>>>> > >>>>>> By the way, the coding style is not configurable, it’s the Google > >> Java > >>>>>> Style, wich uses spaces for indentation. In an IDE or on github the > >>>>>> code looks barely different from the previous formatting at a first > >>>>>> glance. > >>>>>> > >>>>>> For IDE setup, I will add a guide in the README but it boils down > >> to: > >>>>>> > >>>>>> - install the google-java-format plugin, enable it > >>>>>> - install the Save Actions plugin, enable "Optimize Imports" and > >>>>>> "Reformat File" > >>>>>> > >>>>>> With this, every time you save, formatting will be applied > >>>>>> automatically. You will never see formatting complaints from CI > >>>>>> (except for cases where you break semantical checkstyle rules, > >> such as > >>>>>> using certain imports that we don't allow.). > >>>>>> > >>>>>> What do you think? > >>>>>> > >>>>>> Best, > >>>>>> Aljoscha > >>>>>> > >>>>>> On 19.10.20 12:36, Aljoscha Krettek wrote: > >>>>>>> I don't like checkstyle because it cannot be easily applied from > >> the > >>>>>>> commandline. I'm happy to learn otherwise, though. And I'd also be > >>>>>>> very happy about alternative suggestions that can do that. > >>>>>>> > >>>>>>> Right now, I think Spotless is the most straightforward tool for > >>>>>>> that. Also I don't care so much about what style we choose in the > >>>>>>> end. (If we choose one). My main motivation is that we have a > >> common, > >>>>>>> strict style that can easily applied via tooling so that we no > >> longer > >>>>>>> need to comment on coding style in PRs. > >>>>>>> > >>>>>>> Aljoscha > >>>>>>> > >>>>>>> On 09.10.20 11:11, tison wrote: > >>>>>>>> +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 > >>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>>> > >>>>> > >>>>> > >>>> > >>> > >>> > >>> -- > >>> > >>> 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 > >> > >
