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