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. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>> >>> >
