+1
On 01/25/2015 01:23 PM, Pat Ferrel wrote:
I haven’t heard a disagreement. Maybe someone should put the requirements on
the CMS.
This long email includes proposed:
* For Scala follow: http://docs.scala-lang.org/style/
* We are relaxing the required use of infix for *all* appropriate method calls
and allowing “dot” notation. Spark goes further to say you must not use infix
unless it’s an operator method.
* Line lengths not to exceed 120 chars—Spark limits to 100
I'm all for using infix as little as possible. It really does make
scala unnecessarily difficult to read for those coming from other
languages.
I think we're really only using using infix (aside from operators) in
tests where it seems useful and relatively straightforward. eg:
InCore(dslCat2, 3) - aggInCore(dslCat2, 3) should be < epsilon
This is so close to Spark’s guidelines here:
https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide that
maybe we should just reference them.
On Jan 25, 2015, at 9:21 AM, Andrew Musselman <[email protected]>
wrote:
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.
