Of course we should be attentive to style, agree review of style should be a prerequisite to sign off on a PR or commit:
On Saturday, January 24, 2015, Dmitriy Lyubimov <[email protected]> wrote: > Pat, > > I have no (particular) clue what you are talking about. All I am asking is > for committers not to let in commits containing 170 character long lines if > possible please. Or zigzagy shaped comments. > > There are people on this project that used to ask for no trailing > whitespaces that one can't even see unless review board highlights them. Or > that style plugins produce no new warnings. I am not one of them. I just > ask to be able to look at the stuff without turning my screen inside out. > As it has been pointed before by other people in style discussions, too > obvious style sloppiness is not adding to code credibility. Hope that much > is still agreeable. > > Other than that, I don't object any reasonably adopted style guidelines. > > Although as I mentioned, as a spark contributor, I prefer not to deviate > from a single set of rules already set for spark code base, although have > no strong opinion about it as I don't think anybody else here hacks both at > the same time to care enough. > > I dont intend to sound preachy on the subject but please note that there is > difference between committer and contributor roles. Committer is > specifically charged with style and coherence checks, and doing general > code vetting for quality regardless of point of origin of such code. The > fact that we have lines longer than 120 characters in the code base > directly means lapse in committer duties. Using java style coding in Scala > is also one thing that should trigger committee red flags. > > Imo we should try to do it better, shouldn't we? Restore quality peer > reviews practice? > The comments are formatted using IDE defaults, I agree they can be cleaned > up. > > The Scala docs here: > http://docs.scala-lang.org/style/method-invocation.html > are the usual source of guidance. There have been a couple exceptions to > these guidelines including: > 1) We (in much of the mahout scala code I’ve seen) don’t seem to follow the > "no dot” infix style recommended in the guide > http://docs.scala-lang.org/style/method-invocation.html There was a > discussion of this some time ago and seemed be consensus for a more > java-like form. > > As to removing org.apache.mahout.common.Pair this seems like a good idea in > light of the recent refactoring. I took it from a quick back of the > envelope function that Sebastian wrote over a year ago when it was a > non-issue. I’ll look at that the next time I’m in the code. I assume this > would remove the need for Pair being in the module replacing mr-leagacy for > Scala? > > On Jan 24, 2015, at 5:10 AM, Gokhan Capan <[email protected] > <javascript:;>> wrote: > > +1 for favoring native scala types. > > I think in terms of Scala code, we need a clear style standards definition > to adhere to. > > > Gokhan > > On Fri, Jan 23, 2015 at 9:38 PM, Dmitriy Lyubimov <[email protected] > <javascript:;>> wrote: > > > in TextDelimitedReaderWriter.scala: > > > > =========================== > > val itemList: > > collection.mutable.MutableList[org.apache.mahout.common.Pair[Integer, > > Double]] = new > > collection.mutable.MutableList[org.apache.mahout.common.Pair[Integer, > > Double]] > > for (ve <- itemVector.nonZeroes) { > > val item: org.apache.mahout.common.Pair[Integer, Double] = new > > org.apache.mahout.common.Pair[Integer, Double](ve.index, ve.get) > > itemList += item > > } > > ================================ > > > > (1) why scala code attempts to use commons.pair? What was wrong about > > native Tuple type of scala? (I am trying to clean out mrlegacy > dependencies > > from spark module). > > > > (2) why it is so horribly styled (even for me)? comments are misaligned, > > the lines routinely exceed 120 characters? > > > > Can these problems please be addressed? in particular, stuff like > > o.a.m.common.Pair? And why it is even signed off on in the first place by > > committers despite of clear style violations? > > > > thank you. > > >
