So, there are a few modes of checkstyle failure that can be induced by this:
- lines get too long because of wrap & indent logic - a lot of our HTML is actually already broken and doesn't have <p> tags where it should; spotless adds a blank line which makes checkstyle notice the errors I think that autoformat outweighs these. I propose that we comment out these checkstyle rules, turn on autoformat, then gradually restore the rules. I have adjusted my pull request accordingly. Kenn On Wed, Jun 27, 2018 at 5:05 PM Robert Burke <rob...@frantil.com> wrote: > +1! > Given this is the recommended default for Go projects (with it's own gofmt > tool) I'm for similar tooling for this in other languages. > > I suspect we can add Gradle command to run gofmt over the go code too for > a consistent hook to run for beam code. It would save folks from needing to > set up their IDEs for every language independently, any of which they may > not be used to the idiomatic conventions. > > On Wed, Jun 27, 2018, 3:32 PM Kenneth Knowles <k...@google.com> wrote: > >> OK, I opened https://github.com/apache/beam/pull/5797. This thread is >> still less than a day, so there's no commitment yet. There's no big hurry - >> I can always discard the autoformat and rerun it. I'm sure there will >> always be conflicts so I will just have to rerun it and merge at some quiet >> period for the repo. I will see about pinging existing PRs. >> >> On Wed, Jun 27, 2018 at 11:31 AM Daniel Oliveira <danolive...@google.com> >> wrote: >> >>> +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 >>>>>>>>>>>>>> >>>>>>>>>>>>>>
