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
