+1! Given this is the recommended default for Go projects (with it's own gofmt tool) I'm for similar tooling for this in other languages.
I suspect we can add Gradle command to run gofmt over the go code too for a consistent hook to run for beam code. It would save folks from needing to set up their IDEs for every language independently, any of which they may not be used to the idiomatic conventions. On Wed, Jun 27, 2018, 3:32 PM Kenneth Knowles <k...@google.com> wrote: > OK, I opened https://github.com/apache/beam/pull/5797. This thread is > still less than a day, so there's no commitment yet. There's no big hurry - > I can always discard the autoformat and rerun it. I'm sure there will > always be conflicts so I will just have to rerun it and merge at some quiet > period for the repo. I will see about pinging existing PRs. > > On Wed, Jun 27, 2018 at 11:31 AM Daniel Oliveira <danolive...@google.com> > wrote: > >> +1 I'll throw in my support for auto-formatting, especially if the entire >> project is auto-formatted in advance. >> >> On Wed, Jun 27, 2018 at 10:53 AM Huygaa Batsaikhan <bat...@google.com> >> wrote: >> >>> +1. Global auto-formatting is cool! >>> >>> On Wed, Jun 27, 2018 at 10:17 AM Kenneth Knowles <k...@google.com> wrote: >>> >>>> I just mean that because of how the tool works. But I guess if there >>>> were discretion then two different people could end up with autoformatting >>>> that disagrees, so again you get lines in the PR diff that aren't real >>>> changes. >>>> >>>> Kenn >>>> >>>> On Wed, Jun 27, 2018 at 10:16 AM Raghu Angadi <rang...@google.com> >>>> wrote: >>>> >>>>> On Wed, Jun 27, 2018 at 10:13 AM Kenneth Knowles <k...@google.com> >>>>> wrote: >>>>> >>>>>> Nope! No discretion allowed :-) >>>>>> >>>>> >>>>> +1. Fair enough! >>>>> >>>>> >>>>>> >>>>>> On Wed, Jun 27, 2018 at 9:57 AM Raghu Angadi <rang...@google.com> >>>>>> wrote: >>>>>> >>>>>>> +1. >>>>>>> >>>>>>> Wondering if it can be configured to reformat only what we care most >>>>>>> about (2 space indentation etc), allowing some discretion on the edges. >>>>>>> An >>>>>>> example of inconsistent formatting that ends up in my code: >>>>>>> --- >>>>>>> anObject.someLongMethodName(arg_number_1, >>>>>>> arg_number_2); >>>>>>> --- vs --- >>>>>>> anObject.anotherMethodName( >>>>>>> arg_number_1, >>>>>>> arg_number_2 >>>>>>> ); >>>>>>> >>>>>>> >>>>>>> On Wed, Jun 27, 2018 at 9:41 AM Lukasz Cwik <lc...@google.com> >>>>>>> wrote: >>>>>>> >>>>>>>> It wasn't clear to me that the intent was to autoformat all the >>>>>>>> code from the proposal initially. If thats the case, then the delta is >>>>>>>> quite small typically. >>>>>>>> >>>>>>>> Also, it would be easier if we recommended to users to run run >>>>>>>> "./gradlew spotlessApply" which will run spotless on all modules. >>>>>>>> >>>>>>>> On Wed, Jun 27, 2018 at 9:31 AM Kenneth Knowles <k...@google.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Luke: the proposal here solves exactly what you are talking about. >>>>>>>>> >>>>>>>>> The problem you describe happens when the PR author uses >>>>>>>>> autoformat but the baseline is not already autoformatted. What I am >>>>>>>>> proposing is to make sure the baseline is already autoformatted, so >>>>>>>>> PRs >>>>>>>>> never have extraneous formatting changes. >>>>>>>>> >>>>>>>>> Rafael: the default setting on GitHub is "allow edits by >>>>>>>>> maintainers" so actually a committer can run spotless on behalf of a >>>>>>>>> contributor and push the fixup. I have done this. It also lets a >>>>>>>>> committer fix up a good PR and merge it even if the contributor is, >>>>>>>>> say, >>>>>>>>> asleep. >>>>>>>>> >>>>>>>>> Kenn >>>>>>>>> >>>>>>>>> On Wed, Jun 27, 2018 at 9:24 AM Rafael Fernandez < >>>>>>>>> rfern...@google.com> wrote: >>>>>>>>> >>>>>>>>>> Luke: Anything that helps contributors and reviewers work better >>>>>>>>>> together - +1! :D >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <lc...@google.com> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> If spotless is run against a PR that is already well formatted >>>>>>>>>>> its a non-issue as the formatting changes are usually related to >>>>>>>>>>> the change >>>>>>>>>>> but I have reviewed a few PRs that have 100s of lines of formatting >>>>>>>>>>> change >>>>>>>>>>> which really obfuscates the work. >>>>>>>>>>> Instead of asking contributors to run spotless, can we have a >>>>>>>>>>> cron job run it across the project like once a day/week/... and cut >>>>>>>>>>> a PR? >>>>>>>>>>> >>>>>>>>>>> On Wed, Jun 27, 2018 at 8:07 AM Kenneth Knowles <k...@google.com> >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> Good points, Dan. Checkstyle will still run, but just focused >>>>>>>>>>>> on the things that go beyond format. >>>>>>>>>>>> >>>>>>>>>>>> Kenn >>>>>>>>>>>> >>>>>>>>>>>> On Wed, Jun 27, 2018 at 8:03 AM Etienne Chauchot < >>>>>>>>>>>> echauc...@apache.org> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> +1 ! >>>>>>>>>>>>> It's my custom to avoid reformatting to spare meaningless diff >>>>>>>>>>>>> burden to the reviewer. Now it will be over, thanks. >>>>>>>>>>>>> >>>>>>>>>>>>> Etienne >>>>>>>>>>>>> >>>>>>>>>>>>> Le mardi 26 juin 2018 à 21:15 -0700, Kenneth Knowles a écrit : >>>>>>>>>>>>> >>>>>>>>>>>>> Hi all, >>>>>>>>>>>>> >>>>>>>>>>>>> I like readable code, but I don't like formatting it myself. >>>>>>>>>>>>> And I _really_ don't like discussing in code review. "Spotless" >>>>>>>>>>>>> [1] can >>>>>>>>>>>>> enforce - and automatically apply - automatic formatting for >>>>>>>>>>>>> Java, Groovy, >>>>>>>>>>>>> and some others. >>>>>>>>>>>>> >>>>>>>>>>>>> This is not about style or wanting a particular layout. This >>>>>>>>>>>>> is about automation, contributor experience, and streamlining >>>>>>>>>>>>> review >>>>>>>>>>>>> >>>>>>>>>>>>> - Contributor experience: MUCH better than checkstyle: error >>>>>>>>>>>>> message just says "run ./gradlew :beam-your-module:spotlessApply" >>>>>>>>>>>>> instead >>>>>>>>>>>>> of telling them to go in and manually edit. >>>>>>>>>>>>> >>>>>>>>>>>>> - Automation: You want to use autoformat so you don't have to >>>>>>>>>>>>> format code by hand. But if you autoformat a file that was in >>>>>>>>>>>>> some other >>>>>>>>>>>>> format, then you touch a bunch of unrelated lines. If the file is >>>>>>>>>>>>> already >>>>>>>>>>>>> autoformatted, it is much better. >>>>>>>>>>>>> >>>>>>>>>>>>> - Review: Never talk about code formatting ever again. A PR >>>>>>>>>>>>> also needs baseline to already be autoformatted or formatting >>>>>>>>>>>>> will make it >>>>>>>>>>>>> unclear which lines are really changed. >>>>>>>>>>>>> >>>>>>>>>>>>> This is already available via applyJavaNature(enableSpotless: >>>>>>>>>>>>> true) and it is turned on for SQL and our buildSrc gradle >>>>>>>>>>>>> plugins. It is >>>>>>>>>>>>> very nice. There is a JIRA [2] to turn it on for the hold code >>>>>>>>>>>>> base. >>>>>>>>>>>>> Personally, I think (a) every module could make a different >>>>>>>>>>>>> choice if the >>>>>>>>>>>>> main contributors feel strongly and (b) it is objectively better >>>>>>>>>>>>> to always >>>>>>>>>>>>> autoformat :-) >>>>>>>>>>>>> >>>>>>>>>>>>> WDYT? If we do it, it is trivial to add it module-at-a-time or >>>>>>>>>>>>> globally. If someone conflicts with a massive autoformat commit, >>>>>>>>>>>>> they can >>>>>>>>>>>>> just keep their changes and autoformat them and it is done. >>>>>>>>>>>>> >>>>>>>>>>>>> Kenn >>>>>>>>>>>>> >>>>>>>>>>>>> [1] >>>>>>>>>>>>> https://github.com/diffplug/spotless/tree/master/plugin-gradle >>>>>>>>>>>>> [2] https://issues.apache.org/jira/browse/BEAM-4394 >>>>>>>>>>>>> >>>>>>>>>>>>>
