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 <[email protected]> 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 <[email protected]> > wrote: > >> +1. Global auto-formatting is cool! >> >> On Wed, Jun 27, 2018 at 10:17 AM Kenneth Knowles <[email protected]> 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 <[email protected]> >>> wrote: >>> >>>> On Wed, Jun 27, 2018 at 10:13 AM Kenneth Knowles <[email protected]> >>>> wrote: >>>> >>>>> Nope! No discretion allowed :-) >>>>> >>>> >>>> +1. Fair enough! >>>> >>>> >>>>> >>>>> On Wed, Jun 27, 2018 at 9:57 AM Raghu Angadi <[email protected]> >>>>> 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 <[email protected]> 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 <[email protected]> >>>>>>> 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 < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> Luke: Anything that helps contributors and reviewers work better >>>>>>>>> together - +1! :D >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Jun 27, 2018 at 9:04 AM Lukasz Cwik <[email protected]> >>>>>>>>> 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 <[email protected]> >>>>>>>>>> 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 < >>>>>>>>>>> [email protected]> 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 >>>>>>>>>>>> >>>>>>>>>>>>
