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 <[email protected]> wrote: > This sounds really good to me as well. +1 > > On Wed, Oct 12, 2016 at 4:13 PM, Jared Stewart <[email protected]> > 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 <[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. > > >>>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>> > > >>>> > > >> > > > > > > > > > > -- > > ~/William >
