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