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