+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 >>
