I'm in favor of strict coding styles. And I like the google style. But I can adapt. 😀 On Mar 13, 2015 11:26 PM, "Henry Saputra" <henry.sapu...@gmail.com> wrote:
> Agree. > > We have make decision either to play tight or loose on the code style and > guide. > Once the codebase is getting too large and more committers coming in > then it would be too late. > > We can not have our cake and eat it too. > > Looking forward to what others think since I already have my 2-cents out =) > > - Henry > > On Fri, Mar 13, 2015 at 2:31 PM, Fabian Hueske <fhue...@gmail.com> wrote: > > I personally find the Google code style to be too strict/detailed. > > Loosening it by dropping certain rules makes only sense if the deviation > > does not become to large. > > > > My major concern with adding a such strict code style is that all open > PRs > > would become invalid. > > We could try to reduce that effect by adding the code styling module by > > module and primarily resolving PRs that address the next module. > > However, this would still be a huge effort and I am not sure if it would > > pay back. > > > > In any case, it is good to have this discussion now. > > Postponing the decision will make it only more costly if we agree on a > more > > rigid code style. > > > > > > 2015-03-13 20:18 GMT+01:00 Henry Saputra <henry.sapu...@gmail.com>: > > > >> Regarding style, yes, we already have them in place but they are very > >> loose, especially in Java. > >> > >> I guess it is a "no good deed goes unpunished" scenario. To tighten up > >> the style rules, for example following Google Java style with some > >> documented exceptions, will require massive code changes. > >> But we already do large code changes or refactoring in some parts of > >> the code tree. Some current PRs are already huge in size. > >> > >> > >> - Henry > >> > >> On Fri, Mar 13, 2015 at 11:51 AM, Stephan Ewen <se...@apache.org> > wrote: > >> > We already have checkstyle for Java and Scala in place (with marking > >> > violations a breaking the build). > >> > > >> > The rules in Java are very loose, though. We may make them stricter. > >> Would > >> > require extensive passes over a lot of code, though, to fix this. > >> > > >> > The other things (choice of library) seem to be well addressable and > we > >> can > >> > document and enforce them immediately, if we want. > >> > > >> > Stephan > >> > > >> > > >> > On Mon, Mar 9, 2015 at 6:26 PM, Henry Saputra < > henry.sapu...@gmail.com> > >> > wrote: > >> > > >> >> Thanks for bringing up the discussions, Stephan. > >> >> > >> >> Ufuk and Till had brought up some ideas to solve the example of > issues > >> >> you mentioned in the original thread. > >> >> So In the nutshell, we need to have more strick style and rules > >> >> checking for the code to help contributors to submit code change and > >> >> maintain bit more homogenous in style and code flow. > >> >> > >> >> Some ideas of concrete steps I could think of and have done it in > >> >> other projects: > >> >> 1. Add Google Java code style [1] as the checkstyle rule and document > >> >> the differences from the rule (For example, tabs instead of spaces). > >> >> Set it as break build if style is violated. We did similar thing in > >> >> Tachyon [2] and seemed to help. > >> >> 2. Declare Scala code style [3] as the main code style for Scala > >> >> portion of the code and enforce it. Similar to Java add documentation > >> >> for differences or details that is not covered by the rules. > >> >> 3. Add code guide for stuff which are not covered by the rules, such > >> >> as parameter checking to use Guava or common-lang3 and enforce it by > >> >> code review. With this do global code change to reflect this. > >> >> > >> >> Just my 2-cents, and as the others had mentioned, we need to make > this > >> >> as explicit > >> >> > >> >> - Henry > >> >> > >> >> [1] > https://google-styleguide.googlecode.com/svn/trunk/javaguide.html > >> >> [2] > http://tachyon-project.org/Startup-Tasks-for-New-Contributors.html > >> >> [3] http://docs.scala-lang.org/style/ > >> >> > >> >> On Mon, Mar 9, 2015 at 2:35 AM, Till Rohrmann < > till.rohrm...@gmail.com> > >> >> wrote: > >> >> > I also agree that we have too many different ways of doing things. > A > >> set > >> >> of > >> >> > common rules/ways would definitely be beneficial for the project. > >> >> > > >> >> > Concerning the command line parsing: I thought that Alexander > >> Alexandrov > >> >> > wanted to unify the command line parsing by replacing both tools > with > >> a > >> >> > better one. So this should be fixed sooner or later. > >> >> > > >> >> > The different code styles are just natural if there is no common > set > >> of > >> >> > established rules, which are also enforced. I see two solutions: > >> Either > >> >> > enforcing common coding rules or refraining from reformatting the > code > >> >> from > >> >> > other contributors. I don't know whether we can find a common > >> denominator > >> >> > with which everyone can live and which is yet specific enough to > make > >> the > >> >> > code base more homogenous. > >> >> > > >> >> > I also agree that the mixed Java/Scala projects make it harder to > get > >> >> > started. I've often seen that people confuse the basic types (Scala > >> >> tuples > >> >> > vs. our own tuples, Java list vs. Scala list, etc.). This is > probably > >> >> > something we cannot fix without rewriting parts which are > implemented > >> in > >> >> > the "other" language, though. > >> >> > > >> >> > Personally I don't see the different testing styles as critical. > >> Whether > >> >> > one is using JUnit tests, WordSpecLike tests or FlatSpec tests, it > >> should > >> >> > be pretty obvious for everyone where the testing code is written in > >> case > >> >> > that they want to change something. Moreover, most of the time you > >> would > >> >> > not want to change proper defined test cases. In fact, I think that > >> >> > WordSpecLike and FlatSpec let you write better tests, because it > >> >> encourages > >> >> > people to clearly and more naturally write what they want to test. > >> >> Compared > >> >> > to some shorter and often not so meaningful junit method names, > this > >> is > >> >> for > >> >> > me a clear advantage. If a test case fails, I want to directly know > >> >> without > >> >> > having to go through the testing code, what was tested and what > went > >> >> > probably wrong. However, this holds only true if we don't use the > >> >> > JUnitRunner to run these tests. Unfortunately, this is currently > the > >> >> case. > >> >> > With JUnitRunner, the actual result output of the tests is often > hard > >> to > >> >> > decode. Therefore, we should probably stick to writing testing > >> methods if > >> >> > we use JUnit to run our Scala tests but use the more expressive > >> FlatSpec > >> >> > for pure Scala modules. > >> >> > > >> >> > That said, I would be in favour of some explicitly stated > guidelines, > >> >> > because "Lets' keep it in mind" will be forgotten soon. > >> >> > > >> >> > > >> >> > > >> >> > > >> >> > > >> >> > On Mon, Mar 9, 2015 at 8:46 AM, Ufuk Celebi <u...@apache.org> > wrote: > >> >> > > >> >> >> Hey Stephan, > >> >> >> > >> >> >> On 08 Mar 2015, at 23:17, Stephan Ewen <se...@apache.org> wrote: > >> >> >> > >> >> >> > Hi everyone! > >> >> >> > > >> >> >> > I would like to start an open discussion about some issue with > the > >> >> >> > heterogeneity of the Flink code base. > >> >> >> > >> >> >> Thanks for bringing this up. I agree with your position. The > related > >> >> >> discussion about using Guava vs. Validate is a good step into the > >> right > >> >> >> direction. In general, I think it's super hard to get more > >> homogeneity > >> >> >> without enforcing rules (like in the Guava/Validate discussion). I > >> >> would be > >> >> >> OK with trying to settle on rules and then enforcing them. But I'm > >> not > >> >> sure > >> >> >> whether that is what you are asking for? Are you more aiming at a > >> "Let's > >> >> >> keep it in mind" kind of thingy?! > >> >> >> > >> >> >> > Here are a few examples: > >> >> >> > > >> >> >> > - Parameter checking is sometimes done with commons-lang3, > >> >> commons-lang, > >> >> >> > or guava > >> >> >> > - Command line parsing is sometimes done with commons-cli, > >> sometimes > >> >> with > >> >> >> > scopt. > >> >> >> > >> >> >> I think these are easily enforceable and could also be changed > >> manually > >> >> >> w/o too much hassle. > >> >> >> > >> >> >> > - Code styles are quite different from commit to commit. Spaces, > >> >> >> > indentations, braces. Not a critical thing, but seems to > encourage > >> >> people > >> >> >> > to reformat other people's code, whenever the pass over it, > which > >> >> should > >> >> >> be > >> >> >> > avoided (cluttered diffs, may introduce new bugs actually) > >> >> >> > >> >> >> This is something we could more strictly enforce in pull requests > and > >> >> >> generally ask people to refrain from. > >> >> >> > >> >> >> > - Some projects are mixed Java/Scala, which is not perfectly > >> >> supported by > >> >> >> > the tools so far. It also needs many "fromJava / toJava" > >> conversions > >> >> and > >> >> >> > makes the entry hurdle into the project higher. > >> >> >> > - Tests are sometimes written as Java Unit tests, sometimes as > >> Scala > >> >> Unit > >> >> >> > tests (method style), sometimes as Scala Unit Tests (grammar > >> style). > >> >> >> > >> >> >> This is an artifact of the mixed Scala/Java discussion. I agree > that > >> >> this > >> >> >> can be problematic, but I'm not sure how to solve this as long as > we > >> mix > >> >> >> Java/Scala in the same modules?! For new code in the runtime, we > >> could > >> >> >> stick to one language. What do you propose here as a solution? > >> >> >> > >> >> >> > I am eager to hear opinions! > >> >> >> > >> >> >> As I've said, I agree with your points, but I think a big issue > for > >> new > >> >> >> comers and committers alike is missing documentation in the code. > We > >> >> should > >> >> >> try to keep the discussion we had in that regard in mind as well. > >> >> >> > >> >> >> – Ufuk > >> >> > >> >
