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