+1 On Thu, Oct 13, 2016 at 10:04 AM Kevin Duling <kdul...@pivotal.io> wrote:
> Given that, +1 from me! > > On Thu, Oct 13, 2016 at 9:51 AM, Jared Stewart <jstew...@pivotal.io> > wrote: > > > The task is fully suppressible with -x spotlessCheck. Also, if you have > > any formatter errors you can automatically fix them with 'gradle > > spotlessApply’. > > > > > On Oct 13, 2016, at 9:40 AM, Kevin Duling <kdul...@pivotal.io> wrote: > > > > > > If we made formatting a warning, then people would probably quickly > > ignore > > > it. > > > If we made formatting an error, we need to be sure we don't get in to > the > > > situation where <editor of choice>'s formatter is not in agreement with > > the > > > build's checker. > > > > > > I can live with an additional 17 seconds as well. And Jared's already > > > reduced the build time locally by 50%. But I still want the ability to > > > suppress the check similar to -x javadoc. > > > > > > On Wed, Oct 12, 2016 at 9:58 PM, William Markito <wmark...@pivotal.io> > > > wrote: > > > > > >> This sounds really good to me as well. +1 > > >> > > >> On Wed, Oct 12, 2016 at 4:13 PM, Jared Stewart <jstew...@pivotal.io> > > >> wrote: > > >> > > >>> This is running locally on my laptop. Since Spotless is only doing > > code > > >>> formatting and not any other static analysis, it already has 0 > errors. > > >>> (Other than, of course, formatting not consistent with the template.) > > >>> > > >>>> On Oct 12, 2016, at 4:11 PM, Kenneth Howe <kh...@pivotal.io> wrote: > > >>>> > > >>>> Agree with Mark, this has to work with 0 errors before it would be > > >>> useful in precheckin. I think I could live with an additional 17 > > seconds > > >>> most of the time for running the spotlessCheck as suggested. > > >>>> > > >>>> Jared, Is that 17 seconds running locally on your laptop or on a > more > > >>> capable machine? > > >>>> > > >>>> Ken > > >>>> > > >>>>> On Oct 12, 2016, at 3:39 PM, Jared Stewart <jstew...@pivotal.io> > > >> wrote: > > >>>>> > > >>>>> If you want to try it out, I pushed a branch to my Geode repo that > > >>> contains this change: > > >>>>> > https://github.com/jaredjstewart/incubator-geode/tree/spotlessPlugin > > >> < > > >>> https://github.com/jaredjstewart/incubator-geode/tree/spotlessPlugin > > > > >>>>> > > >>>>>> On Oct 12, 2016, at 2:27 PM, Darrel Schneider < > > dschnei...@pivotal.io > > >>> > > >>> wrote: > > >>>>>> > > >>>>>> I like Dan's idea of catching formatting issues earlier but I > think > > >>> adding > > >>>>>> 5-10 minutes to "build" would be too much. Currently when I'm > trying > > >>> to do > > >>>>>> a quick build I use -xjavadoc. I'd probably do the same for this > > >>> target if > > >>>>>> it was part of build until I'm ready to do a precheckin. > > >>>>>> > > >>>>>> Mark, wouldn't running the formatter on all our java files and > > >> checking > > >>>>>> them in get these issues down to 0? > > >>>>>> > > >>>>>> On Wed, Oct 12, 2016 at 12:53 PM, Udo Kohlmeyer < > > >> ukohlme...@pivotal.io > > >>>> > > >>>>>> wrote: > > >>>>>> > > >>>>>>> +1 - adding checkstyle to precheckin. > > >>>>>>> > > >>>>>>> If the developer uses the provided templates ( eclipse + > intellij) > > >>> then > > >>>>>>> most of the formatting issues should be handled before > precheckin. > > >>> Also, if > > >>>>>>> a developer has a questionable coding style, that should lessen > as > > >>> that > > >>>>>>> developer will have resolve the issues before being able to > commit. > > >>>>>>> > > >>>>>>> I also believe that this should not be an overbearing and > intrusive > > >>>>>>> process. > > >>>>>>> > > >>>>>>> --Udo > > >>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>> On 13/10/16 6:36 am, Mark Bretl wrote: > > >>>>>>> > > >>>>>>>> Dan, > > >>>>>>>> > > >>>>>>>> There is some extra amount of time, 5-10 minutes extra for the > > >> entire > > >>>>>>>> project (depending on your CPU). I think the real issue to > adding > > >> it > > >>> to > > >>>>>>>> the > > >>>>>>>> precheckin target and have it be 'effective' is it needs to run > > >>>>>>>> successfully, otherwise it would turn into noise most of the > time > > I > > >>> think. > > >>>>>>>> We need to get the issues down to 0 or manage to set a new > > baseline > > >>> (not > > >>>>>>>> the best idea), which is a lot of work, to make it run > > >> successfully. > > >>> Right > > >>>>>>>> now, if you run the target, it will fail every time since there > > >>>>>>>> outstanding > > >>>>>>>> issues in the code and very hard to tell what issues were > > >> introduced. > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> --Mark > > >>>>>>>> > > >>>>>>>> On Wed, Oct 12, 2016 at 11:34 AM, Dan Smith <dsm...@pivotal.io> > > >>> wrote: > > >>>>>>>> > > >>>>>>>> Seems like it should run as part of the build target. The only > > >>> reason to > > >>>>>>>>> make it part of precheckin is if it takes a long time, > otherwise > > >>> people > > >>>>>>>>> should get fast feedback they need to change their code before > > >> they > > >>> push. > > >>>>>>>>> > > >>>>>>>>> -Dan > > >>>>>>>>> > > >>>>>>>>> On Wed, Oct 12, 2016 at 11:24 AM, Jared Stewart < > > >>> jstew...@pivotal.io> > > >>>>>>>>> wrote: > > >>>>>>>>> > > >>>>>>>>> +1 to running during the precheckin target as well as Travis CI > > >>>>>>>>>> > > >>>>>>>>>> On Oct 12, 2016 11:20 AM, "Darrel Schneider" < > > >>> dschnei...@pivotal.io> > > >>>>>>>>>> wrote: > > >>>>>>>>>> > > >>>>>>>>>> If Travis CI is only run on pull requests then that is not > > enough > > >>>>>>>>>>> > > >>>>>>>>>> because > > >>>>>>>>> > > >>>>>>>>>> committers do not submit pull requests. Having it run during > the > > >>> gradle > > >>>>>>>>>>> build or precheckin target is also needed. In addition to > that > > I > > >>> also > > >>>>>>>>>>> wanted PRs to be checked. > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>>>> On Wed, Oct 12, 2016 at 11:12 AM, Jared Stewart < > > >>> jstew...@pivotal.io> > > >>>>>>>>>>> wrote: > > >>>>>>>>>>> > > >>>>>>>>>>> It would certainly be necessary to make sure that the code > > style > > >>> to > > >>>>>>>>>>>> > > >>>>>>>>>>> be > > >>>>>>>>> > > >>>>>>>>>> enforced is sensible, e.g. doe not use wildcard imports. We > > >> would > > >>>>>>>>>>>> > > >>>>>>>>>>> also > > >>>>>>>>> > > >>>>>>>>>> want to make one large commit to format all existing code > before > > >>>>>>>>>>>> > > >>>>>>>>>>> turning > > >>>>>>>>>> > > >>>>>>>>>>> this on. > > >>>>>>>>>>>> > > >>>>>>>>>>>> Mark - Thank you for the information about the existing > setup. > > >>>>>>>>>>>> > > >>>>>>>>>>>> Mark, Darrel, Kevin - Given all of your comments, I think it > > >>> might > > >>>>>>>>>>>> > > >>>>>>>>>>> make > > >>>>>>>>> > > >>>>>>>>>> more sense to add the flag to enable it in Travis CI rather > than > > >> as > > >>>>>>>>>>>> > > >>>>>>>>>>> part > > >>>>>>>>>> > > >>>>>>>>>>> of the build. This way your build pass regardless of > > >> whitespace, > > >>>>>>>>>>>> > > >>>>>>>>>>> but > > >>>>>>>>> > > >>>>>>>>>> the > > >>>>>>>>>>> > > >>>>>>>>>>>> CI job would fail on PRs if they did not adhere to the > > standard > > >>>>>>>>>>>> > > >>>>>>>>>>> formatting. > > >>>>>>>>>>> > > >>>>>>>>>>>> Anthony - It doesn’t seem to me that turning this on would > > have > > >>> the > > >>>>>>>>>>>> > > >>>>>>>>>>> effect > > >>>>>>>>>>> > > >>>>>>>>>>>> of combining reformatting commits and logic changes. > Rather, > > >>> since > > >>>>>>>>>>>> > > >>>>>>>>>>> all > > >>>>>>>>> > > >>>>>>>>>> code would already be formatted, there would no longer be any > > >>>>>>>>>>>> > > >>>>>>>>>>> reformatting > > >>>>>>>>>>> > > >>>>>>>>>>>> commits except for single large commits when the code style > > >> file > > >>> was > > >>>>>>>>>>>> updated. > > >>>>>>>>>>>> > > >>>>>>>>>>>> On Oct 12, 2016, at 11:01 AM, Bruce Schuchardt < > > >>>>>>>>>>>>> > > >>>>>>>>>>>> bschucha...@pivotal.io > > >>>>>>>>>> > > >>>>>>>>>>> wrote: > > >>>>>>>>>>>> > > >>>>>>>>>>>>> I like the idea of doing this but I don't think Checkstyle > > >>> should > > >>>>>>>>>>>>> > > >>>>>>>>>>>> be > > >>>>>>>>> > > >>>>>>>>>> enabled until all of the code is reformatted. > > >>>>>>>>>>>> > > >>>>>>>>>>>>> Also, last time I checked there was still a problem with > the > > >>>>>>>>>>>>> > > >>>>>>>>>>>> IntelliJ > > >>>>>>>>> > > >>>>>>>>>> auto-format settings. It still used wildcard imports, which I > > >>>>>>>>>>>> > > >>>>>>>>>>> believe > > >>>>>>>>> > > >>>>>>>>>> we > > >>>>>>>>>> > > >>>>>>>>>>> don't allow. I've manually changed my settings in > Editor->Code > > >>>>>>>>>>>> > > >>>>>>>>>>> Style->Java > > >>>>>>>>>>> > > >>>>>>>>>>>> to "Use single class import" to correct that problem. I > > >>> couldn't see > > >>>>>>>>>>>> > > >>>>>>>>>>> how > > >>>>>>>>>> > > >>>>>>>>>>> to get Gradle to do it. > > >>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> Le 10/12/2016 à 10:28 AM, Anthony Baker a écrit : > > >>>>>>>>>>>>> > > >>>>>>>>>>>>>> Source code with a consistent look-and-feel makes it > easier > > >> for > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>> people > > >>>>>>>>>> > > >>>>>>>>>>> to join the project community and contribute. > > >>>>>>>>>>>> > > >>>>>>>>>>>>> Let’s continue to keep reformatting commits separate from > > >> logic > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>> changes—otherwise it’s too hard to review. > > >>>>>>>>>>>> > > >>>>>>>>>>>>> Anthony > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> On Oct 12, 2016, at 10:06 AM, Dan Smith < > dsm...@pivotal.io> > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>> wrote: > > >>>>>>>>> > > >>>>>>>>>> +1 > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> This might be a good time to reformat the code since I > > don't > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>> think > > >>>>>>>>> > > >>>>>>>>>> there > > >>>>>>>>>>>> > > >>>>>>>>>>>>> are too many long lived feature branches outstanding. > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> -Dan > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> On Wed, Oct 12, 2016 at 10:00 AM, Jared Stewart < > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>>>> jstew...@pivotal.io > > >>>>>>>>>> > > >>>>>>>>>>> wrote: > > >>>>>>>>>>>> > > >>>>>>>>>>>>> I would like to advocate for adding a Checkstyle < > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> http://checkstyle > > >>>>>>>>>> > > >>>>>>>>>>> . > > >>>>>>>>>>> > > >>>>>>>>>>>> sourceforge.net/> or Spotless <https://github.com/diffplug/ > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> spotless > > >>>>>>>>>> > > >>>>>>>>>>> gradle task to our build process to ensure that all code > > checked > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> in > > >>>>>>>>>> > > >>>>>>>>>>> meets > > >>>>>>>>>>>> > > >>>>>>>>>>>>> the formatting standards described on the wiki < > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> https://cwiki.apache.org/ > > >>>>>>>>>>>> > > >>>>>>>>>>>>> confluence/display/GEODE/Code+Style+Guide> (and in the > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> intellij/eclipse > > >>>>>>>>>>>> > > >>>>>>>>>>>>> formatter xml files in our repository). This will alleviate > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> difficulties > > >>>>>>>>>>>> > > >>>>>>>>>>>>> reviewing code when whitespace or formatting has changed > > since > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> all > > >>>>>>>>> > > >>>>>>>>>> code > > >>>>>>>>>>>> > > >>>>>>>>>>>>> checked in will already comply with standards. > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>> > > >>>>> > > >>>> > > >>> > > >>> > > >> > > >> > > >> -- > > >> > > >> ~/William > > >> > > > > >
