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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> >>> 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 <[email protected]> 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 <[email protected]> >>>>>> wrote: >>>>>> >>>>>> +1 to running during the precheckin target as well as Travis CI >>>>>>> >>>>>>> On Oct 12, 2016 11:20 AM, "Darrel Schneider" <[email protected]> >>>>>>> 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 <[email protected]> >>>>>>>> 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 < >>>>>>>>>> >>>>>>>>> [email protected] >>>>>>> >>>>>>>> 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 <[email protected]> >>>>>>>>>>>> >>>>>>>>>>> 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 < >>>>>>>>>>>> >>>>>>>>>>> [email protected] >>>>>>> >>>>>>>> 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. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>> >>>> >> >
