+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
