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