I did some investigation this afternoon on using the Gradle Spotless plugin rather than Checkstyle. Whereas Checkstyle has its own formatting template syntax, Spotless can import our existing Eclipse formatter template. The plugin adds tasks for “spotlessCheck” (which tells you whether or not your code is compliant) and “spotlessApply” (to automatically fix formatting errors for you). By default, “spotlessCheck" runs as part of “build”. Spotless also runs much faster than Checkstyle - it only added roughly 17 seconds to my build.
> 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. >>>>>>>>>>> >>>>>>>>>> >>>>>>> >>
