Re: Coding practices/standards

I'm having trouble with Spotless. spotlessApply on MS-Windows is changing all of the files it touches to have CRLF line separators. Adding endWithNewline() to the java config in build.gradle doesn't help. If I run geode-core:spotlessApply I end up with over 5000 modified files. Le

Re: Coding practices/standards

+1 - Nice work. Thx On 24/10/16 10:40 am, Dan Smith wrote: Doing a spotlessApply on my feature branch before rebasing didn't help bring down the number of conflicts. I came up with this sequence of steps to rebase a feature branch on develop that avoids the need to manually resolve conflicts

Re: Coding practices/standards

This is awesome, thank you for taking the time to figure out how to do this smoothly. —Jared > On Oct 24, 2016, at 10:40 AM, Dan Smith wrote: > > Doing a spotlessApply on my feature branch before rebasing didn't help > bring down the number of conflicts. > > I came up with

Re: Coding practices/standards

Doing a spotlessApply on my feature branch before rebasing didn't help bring down the number of conflicts. I came up with this sequence of steps to rebase a feature branch on develop that avoids the need to manually resolve conflicts with the formatting changes. The trick here is to pick up

Re: Coding practices/standards

Try this commit hash instead: d0175ec5aa8acf1b34ece3183fe03e9874450cbb (from feature/spotlessPlugin). > On Oct 21, 2016, at 4:16 PM, Kirk Lund wrote: > > FYI, feeb5c98402881156b34e222c58ce15c71a4fca7 doesn't exist in the Apache > git repo. > > Is there a way to reformat a

Re: Coding practices/standards

FYI, feeb5c98402881156b34e222c58ce15c71a4fca7 doesn't exist in the Apache git repo. Is there a way to reformat a branch and then rebase on develop to minimize conflicts? -Kirk On Fri, Oct 21, 2016 at 1:36 PM, Jared Stewart wrote: > Fantastic, thanks for merging this in

Re: Coding practices/standards

This looks pretty good indeed! Thanks Jared! On Fri, Oct 21, 2016 at 3:10 PM, Dan Smith wrote: > We've seen the sync to github lag before when there are lots of changes, > like when the docs were contributed. Hopefully the changes should show up > there shortly! > > -Dan > >

Re: Coding practices/standards

We've seen the sync to github lag before when there are lots of changes, like when the docs were contributed. Hopefully the changes should show up there shortly! -Dan On Fri, Oct 21, 2016 at 2:53 PM, Jared Stewart wrote: > By the way, when I pull develop from GitHub I

Re: Coding practices/standards

By the way, when I pull develop from GitHub I don’t see these changes. Nor do I see them when I look here . It seems that they were added to the apache git repo but not the apache GitHub repo, maybe due to the GitHub DNS issues

Re: Coding practices/standards

I haven’t dug in to the configuration of eclipseOrganizeImports, so for now spotless is not enforcing import ordering. If we verify that the import ordering specified by this file looks good, I can turn that on in the spotless config. — Jared > On Oct 21, 2016, at 1:38 PM, Kirk Lund

Re: Coding practices/standards

I just did a pull and now I'm reviewing the formatters under etc/. -rw-rw-r-- 1 klund staff 36004 Oct 21 13:36 eclipse-java-google-style.xml -rw-rw-r-- 1 klund staff110 Sep 19 11:56 eclipseOrganizeImports.importorder -rw-rw-r-- 1 klund staff 20653 Oct 21 13:36

Re: Coding practices/standards

+1 On 21/10/16 1:36 pm, Jared Stewart wrote: Fantastic, thanks for merging this in Mark. For anyone with outstanding work on branches made before this change, your life may be made easier by cherry-picking feeb5c98402881156b34e222c58ce15c71a4fca7 (which added Spotless) into your branch and

Re: Coding practices/standards

Fantastic, thanks for merging this in Mark. For anyone with outstanding work on branches made before this change, your life may be made easier by cherry-picking feeb5c98402881156b34e222c58ce15c71a4fca7 (which added Spotless) into your branch and then running ‘gradlew spotlessApply’ on it

Re: Coding practices/standards

Thanks Jared for the suggestion of Spotless and follow-up work. This is now completed and checked into develop. As this does touch many files, be prepared the next time you pull. --Mark On Fri, Oct 21, 2016 at 1:21 PM, Jared Stewart wrote: > Done! :) > > - Jared > > On

Re: Coding practices/standards

One more time! :) Conflicting files geode-core/src/test/java/org/apache/geode/disttx/PRDistTXDUnitTest.java geode-core/src/test/java/org/apache/geode/disttx/PRDistTXWithVersionsDUnitTest.java geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java --Mark On

Re: Coding practices/standards

I just pulled and rebased onto develop, and force pushed into the existing pull request. It should be clean to merge in now. Thanks, Jared > On Oct 21, 2016, at 11:57 AM, Mark Bretl wrote: > > I believe there is enough consensus here to check this into develop. > >

Re: Coding practices/standards

Sounds like a plan! Let’s do this :-) Anthony > On Oct 21, 2016, at 11:57 AM, Mark Bretl wrote: > > I believe there is enough consensus here to check this into develop. > > Jared, due to recent checkins into develop, can you update the pull request > one more time?

Re: Coding practices/standards

+1 > On Oct 21, 2016, at 8:27 AM, Bruce Schuchardt wrote: > > +1 > > Le 10/20/2016 à 5:13 PM, Udo Kohlmeyer a écrit : >> +1 >> >> >> On 20/10/16 4:56 pm, Mark Bretl wrote: >>> +1 as well... >>> >>> - Pulled changes >>> - Executed './gradlew clean build' and was

Re: Coding practices/standards

+1 Le 10/20/2016 à 5:13 PM, Udo Kohlmeyer a écrit : +1 On 20/10/16 4:56 pm, Mark Bretl wrote: +1 as well... - Pulled changes - Executed './gradlew clean build' and was successful. - Modified a couple of random files to test - Ran './gradlew clean build' again and failed expectedly - Ran

Re: Coding practices/standards

+1 On 20/10/16 4:56 pm, Mark Bretl wrote: +1 as well... - Pulled changes - Executed './gradlew clean build' and was successful. - Modified a couple of random files to test - Ran './gradlew clean build' again and failed expectedly - Ran './gradlew spotlessApply', task was successful - Ran

Re: Coding practices/standards

+1 as well... - Pulled changes - Executed './gradlew clean build' and was successful. - Modified a couple of random files to test - Ran './gradlew clean build' again and failed expectedly - Ran './gradlew spotlessApply', task was successful - Ran './gradlew clean build' and succeeded Great

Re: Coding practices/standards

+1 I just added my approval to the PR (and again here) On Thu, Oct 20, 2016 at 3:25 PM, Jared Stewart wrote: > I have opened a pull request here incubator-geode/pull/268> to enable the Spotless plugin and to switch to > the Google Java Style

Re: Coding practices/standards

For reference TRAC #38741 was a bug with the summary "EOFException during deserialize on client update with copy-on-read=true" -Kirk On Tue, Oct 18, 2016 at 4:27 PM, Jared Stewart wrote: > To give everyone an update, using the Google Java Style eclipse template > there is

Re: Coding practices/standards

To give everyone an update, using the Google Java Style eclipse template there is an issue spotlessCheck where fails for geode-core/src/test/java/org/apache/geode/cache30/Bug38741DUnitTest.java even if you run it directly after spotlessApply. This needs to be investigated and fixed before I

Re: Coding practices/standards

+1 - The formatting looks better now. -Dan On Thu, Oct 13, 2016 at 11:06 AM, Jared Stewart wrote: > I agree that the formatter needs fixing up. Our wiki < > https://cwiki.apache.org/confluence/display/GEODE/Code+Style+Guide> says > that we follow the Google Java Style

Re: Coding practices/standards

+1 The Google style guide is what we chose for Geode when we couldn't use the Elements book Le 10/14/2016 à 4:03 PM, Kirk Lund a écrit : +1 to a) embracing https://google.github.io/styleguide/javaguide.html 100%, b) adopting the google formatters for IntelliJ and Eclipse, c)using spotless

Re: Coding practices/standards

+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 wrote: > +1 > > On Thu, Oct 13, 2016 at 10:04 AM Kevin Duling

Re: Coding practices/standards

+1 On Thu, Oct 13, 2016 at 10:04 AM Kevin Duling wrote: > Given that, +1 from me! > > On Thu, Oct 13, 2016 at 9:51 AM, Jared Stewart > wrote: > > > The task is fully suppressible with -x spotlessCheck. Also, if you have > > any formatter errors you can

Re: Coding practices/standards

I agree that the formatter needs fixing up. Our wiki says that we follow the Google Java Style guide, but that is not actually what’s in our formatter templates. I pushed a new branch

Re: Coding practices/standards

+1 for adding this to ./gradlew build But I think we might want to fix up the formatter a bit before reformatting the code. I tried running spotlessApply, and it did some unfortunate reformatting of code to make it less readable. One problem is with method chaining. We have a few different

Re: Coding practices/standards

Given that, +1 from me! On Thu, Oct 13, 2016 at 9:51 AM, Jared Stewart 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,

Re: Coding practices/standards

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 wrote: > > If we made formatting a warning, then people would probably

Re: Coding practices/standards

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 's formatter is not in agreement with the build's checker. I can live with an additional 17 seconds as well. And Jared's

Re: Coding practices/standards

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

Re: Coding practices/standards

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 wrote: > > Agree

Re: Coding practices/standards

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

Re: Coding practices/standards

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 > On Oct 12, 2016, at 2:27 PM, Darrel Schneider

Re: Coding practices/standards

I did some investigation this afternoon on using the Gradle Spotless plugin rather than Checkstyle. Whereas Checkstyle has its own formatting template syntax, Spotless can import our existing Eclipse formatter template. The plugin adds tasks for “spotlessCheck” (which tells you whether or not

Re: Coding practices/standards

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

Re: Coding practices/standards

+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

Re: Coding practices/standards

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

Re: Coding practices/standards

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

Re: Coding practices/standards

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

Re: Coding practices/standards

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

Re: Coding practices/standards

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,

Re: Coding practices/standards

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

Re: Coding practices/standards

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

Re: Coding practices/standards

Hi Jared, The Checkstyle plugin is already added to the Gradle build. By default it is turned off, however, if you pass the property '-PstaticAnalysis' to the command-line, it will run CheckStyle and FindBugs. It was setup with only basic configuration, so the small amount of work would be to add

Re: Coding practices/standards

I know I've been guilty of submitting code for review that hasn't matched the code style in the past. As a result, I've checked the "reformat code" box on the 'commit' dialog in IDEA, pointing to the IntelliJ xml formatter. It's frustrating to me that this sort of thing is still an issue in

Re: Coding practices/standards

+1 When a pull request is submitted some type of build is automatically done to make sure the code compiles. Would this new "code check" also be done in that automatic check? On Wed, Oct 12, 2016 at 10:07 AM, Nabarun Nag wrote: > +1 > > On Wed, Oct 12, 2016 at 10:06 AM Dan

Re: Coding practices/standards

+1 On Wed, Oct 12, 2016 at 10:06 AM Dan Smith 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 >

Re: Coding practices/standards

+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 wrote: > I would like to advocate for adding a Checkstyle

Coding practices/standards

I would like to advocate for adding a Checkstyle or Spotless gradle task to our build process to ensure that all code checked in meets the formatting standards described on the wiki