One more thing: once we apply this PR, backporting to 0.9.0 will become much harder. This is one of the reasons (the other being that I'd like to see the impact of the rules on the existing codebase and haven't had the time to check) that I haven't commented on your PR in more detail. We are still backporting PRs to 0.9.0 regularly, but maybe this will change in the near future as the important bugs are tackled.
Ismael On Fri, Jan 8, 2016 at 5:33 PM, Ismael Juma <ism...@juma.me.uk> wrote: > Hey Grant, > > As you know I think this is definitely valuable. My take: > > 1. The choice of rules is really important and it's a good plan to be > conservative initially (as you suggested) > 2. We should only use errors, not warnings > 3. To make the previous one feasible, the rules we enable must not have > false positives (or they must be _really_ rare and with a reasonable > workaround). > > Ismael > > On Fri, Jan 8, 2016 at 5:18 PM, Grant Henke <ghe...@cloudera.com> wrote: > >> A while back I did a some preliminary work on KAFKA-2423: Introduce >> ScalaStyle. There is a pull request here: >> https://github.com/apache/kafka/pull/560. The important content for >> review >> is the rule set defined in scalastyle_config.xml. >> >> I know complete consensus on something like this may not be feasible, but >> I >> think there is value in defining some basic style rules to expedite the >> patch review process and improve code readability, clarity, and quality. >> The Java Checkstyle rules added to Kafka have worked well, and I think >> having similar rules for Scala are important. >> >> If there are no major oppositions to the rules, I would like to first: >> >> 1. Add an optional build task to check the code for ScalaStyle rule >> violations >> 2. Fix the very basic violations (whitespace, line length, etc) >> 3. Over time fix more impactful violations >> >> During this time period we can also identify if the rules are too strict, >> annoying or need to be adjusted. Once the issues are fixed we can then run >> ScalaStyle checks as a part of the build to be sure no new violations >> occur. >> >> Do we agree these checks would be valuable? Do the rules and approach >> proposed in my pull request seam reasonable? >> >> Thanks, >> Grant >> -- >> Grant Henke >> Software Engineer | Cloudera >> gr...@cloudera.com | twitter.com/gchenke | linkedin.com/in/granthenke >> > >
