search

typelevel / spire

Powerful new number types and numeric abstractions for Scala.
http://typelevel.org/spire/
MIT License
1.77k stars 243 forks source link

Add and configure scalastyle plugin #450

Open rklaehn opened 9 years ago

rklaehn commented 9 years ago

It would be nice to have an automated way to enforce coding standards. We don't have to have very strict rules, but things like no whitespace at the end of lines should be uncontroversial.

non/cats is using scalastyle, so we could use the scalastyle-config.xml as a starting point. Currently there are many lines with whitespace at the end of the line. These would have to be changed.

rklaehn commented 9 years ago

Using scalastyle-config.xml from non/cats on the core project produces some 1238 errors:

Type Count
Public method must have explicit type 835
Whitespace at end of line 210
Avoid using return 79
Cyclomatic complexity 37
Avoid using null 26

Fixing this would be some work. The return statements are probably there for a reason in most cases. As are the cyclomatic complexities and the nulls. So these warnings would have to be disabled using scalastyle-off:... on a case by case basis.

Removing the whitespace should be uncontroversial.

The question is what to do about "Public method must have explicit type". Disable completely, or just add the types and risk a bit more verbosity? @non @tixxit any opinions?

non commented 9 years ago

So my recommendation here would be:

  1. Disable return, complexity, and null warnings.
  2. Fix whitespace
  3. Add explicit types

I'm sure some of the types will seem obvious, but I feel like the code almost always becomes more readable when the interfaces are explicit.

non commented 9 years ago

In the future we can look into turning on some of the other warnings (or adding our own custom rules) but I think the explicit return types, whitespace, and style issues are what I'm most concerned about right now.

tixxit commented 9 years ago

Agree with what was said above. Explicit types are also better for backwards compatibility - we don't want to accidentally promise a return type that is more specific than it should be.

Definitely a :+1: on adding ScalaStyle though.

rklaehn commented 9 years ago

There are four instances of println in the core code. I guess we should get rid of them:

Rudigers-MBP:spire rklaehn $ sbt -Dsbt.log.noformat=true core/scalastyle | grep println
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/Jet.scala:108:5: Regular expression matched 'println'
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/package.scala:169:8: Regular expression matched 'println'
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/poly/PolyDense.scala:170:12: Regular expression matched 'println'
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/poly/PolyDense.scala:171:12: Regular expression matched 'println'

Replace them with System.error.println or remove completely?

non commented 9 years ago

I see one println in a comment, which is fine.

But yeah, the other prints seem to be leftover debugging code, and should be removed.

EDIT: If we need to change the println in a comment to "fix" scalastyle, let's replace println with something similar that won't be matched by the regex.

rklaehn commented 9 years ago

I agree about having explicit types for stuff that is really public and not just a scalastyle false alarm. And for the few cases of scalastyle false alarms I usually add them anyway to get it to shut up...

rklaehn commented 9 years ago

I fixed most explicit type and other warnings in the most mechanical way possible. Here is what is left:

[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:202:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:203:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:204:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:205:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:206:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:207:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:208:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:209:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:210:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:211:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:215:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:217:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:219:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:221:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:223:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:232:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:234:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:237:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/macros/Auto.scala:238:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/Jet.scala:629:6: Public method must have explicit type
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/Natural.scala:23:22: You should implement equals and hashCode together
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/Number.scala:120:25: You should implement equals and hashCode together
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/Number.scala:269:25: You should implement equals and hashCode together
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/Number.scala:422:25: You should implement equals and hashCode together
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/Number.scala:498:25: You should implement equals and hashCode together
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/Rational.scala:324:15: You should implement equals and hashCode together
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/Rational.scala:748:13: You should implement equals and hashCode together
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/SafeLong.scala:225:11: You should implement equals and hashCode together
[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/SafeLong.scala:378:11: You should implement equals and hashCode together

I am not sure what type I should return in the Auto.scala warnings, since I am not familiar with macros. I also don't want to disable the equals/hashCode warnings since there seems to be something to them.

non commented 9 years ago

You can get the types for ScalaAlgebra and JavaAlgebra methods from the AutoAlgebra class. They will mostly be things like c.Expr[A].

I agree that we should make sure to implement equals and hashCode together, rather than suppressing the warnings.

rklaehn commented 9 years ago

OK. I added the macro types and set equals/hashCode as warning for now (even though I think that there are some cooperative equality/hashcode bugs to be found there). That leaves 

[error] /Users/rklaehn/projects_git/rklaehn/spire/core/src/main/scala/spire/math/Jet.scala:629:6: Public method must have explicit type

I am not sure why nroot returns f and is thus of type Field[T]. You should think that nroot would be of type NRoot[T] and return n.

non commented 9 years ago

That looks like a major bug! Feel free to do whatever is necessary to "fix" it. (I'd probably try commenting it out first -- maybe it is not even necessary).

rklaehn commented 9 years ago

IDEA says that it is not used. I changed it to return n and be of type NRoot[T].

So now core is error free as far as scalastyle is concerned.

non commented 9 years ago

Thanks!