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. > This is a great practice, review the technical part, use the tool to address the mechanical part. > > 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 >>>>> >>>>>
smime.p7s
Description: S/MIME Cryptographic Signature
