+1 to a) embracing https://google.github.io/styleguide/javaguide.html 100%, b) adopting the google formatters for IntelliJ and Eclipse, c)using spotless
On Fri, Oct 14, 2016 at 6:56 AM, Jacob Barrett <[email protected]> wrote: > +1 > > On Thu, Oct 13, 2016 at 10:04 AM Kevin Duling <[email protected]> wrote: > > > Given that, +1 from me! > > > > On Thu, Oct 13, 2016 at 9:51 AM, Jared Stewart <[email protected]> > > wrote: > > > > > The task is fully suppressible with -x spotlessCheck. Also, if you > have > > > any formatter errors you can automatically fix them with 'gradle > > > spotlessApply’. > > > > > > > On Oct 13, 2016, at 9:40 AM, Kevin Duling <[email protected]> > wrote: > > > > > > > > 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 > > > >> > > > > > > > > >
