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