PublicMethodsHaveTypeChecker should not flag nested defs

[scalastyle]

PublicMethodsHaveTypeChecker should not flag nested defs

[leifwickland]

+1

[coltfred]

Agreed! +1

[leifwickland]

I took a stab at trying to fix this. I got far enough to realize that the Scalariform parser doesn't seem to designate function definitions as being nested. It was easy enough to modify traverse() to know if the function it was looking at was declared inside of another function, but that's insufficient because Scala allows a function to declare a type which contains functions which should not be considered nested. I didn't see an obvious way to detect in the stream of Scalariform tokens that a new anonymous type was being defined. Does Scalastyle contain code to do that already?

[lhohan]

+1

[rklaehn]

+1

I am not sure if this can be done in the same turn, but I would also like the following to be ignored:

trait Bla {
  def foo: Int
}

object Bla {
  ...
  private class Impl extends Bla {
    def foo = 3 // this is effectively private, so it should not be flagged!
  }
}

[leifwickland]

@rklaehn

Does the ignoreOverride property satisfy the requirement for the second half of your request?

[rklaehn]

I did not know about this. It mostly satisfies the requirement, but not entirely.

I like to write in a style where I define sealed traits or sealed abstract classes, and then implement the logic in a private class in the companion object of the trait. That way you don't have to put private in front of every little helper method, and it is much more difficult to accidentally make something public. So I would like to have scalastyle ignore everything in a private implementation class, not just overridden methods.

[leifwickland]

@rklaehn

That was supposed to be:

[leifwickland]

@rklaehn Yeah, that makes sense. No, I don't know of a way to configure Scalastyle to do that. It seems like it ought to be possible to detect that situation.

[matthewfarwell]

I think I've fixed this.

Could one of you try it out and see if it fulfils your needs please? You can use the batch mode of scalastyle. Thanks.

As for the private class, I don't think there is anything I can do about this. You say that it is effectively private, but it isn't really. I can't make it private because it implements a trait. If it didn't implement the trait, then I could (should?) make it private.

[leifwickland]

@matthewfarwell, thank you for your work on this.

Your change almost entirely fixes the issue. The one case it doesn't handle could be considered a separate issue.

Specifically if a val receives its value from an anonymous block and a defis defined inside that block then scalastyle still treats it as a public method.

For example in the following, adtToName is detected as violating PublicMethodsHaveTypeChecker:

private val namesToAdt: Map[String, SomeAdt] = {
  def adtToName(a: SomeAdt) = a match {
    case Adt1 | Adt2 | Adt3 => a.name
  }
  AllAdtMembers.map(a => adtToName(a) -> a).toMap
}

If you feel like that should be treated as a separate issue, please let me know. I'd be happy to write it up.

Thanks again for your hard work fixing this issue.

[matthewfarwell]

Yes, this should be raised as a separate issue. It is a relatively easy
fix, which I'll do when I get a minute.

Have fun,

Matthew Farwell.

2014-12-29 21:01 GMT+01:00 Leif Wickland [email protected]:

@matthewfarwell https://github.com/matthewfarwell, thank you for your
work on this.

Your change almost entirely fixes the issue. The one case it doesn't
handle could be considered a separate issue.

Specifically if a val receives its value from an anonymous block and a defis
defined inside that block then scalastyle still treats it as a public
method.

For example in the following, adtToName is detected as violating
PublicMethodsHaveTypeChecker:

private val namesToAdt: Map[String, SomeAdt] = {
def adtToName(a: SomeAdt) = a match {
case Adt1 | Adt2 | Adt3 => a.name
}
AllAdtMembers.map(a => adtToName(a) -> a).toMap
}

If you feel like that should be treated as a separate issue, please let me
know and I'll write up up.

Thanks again for your hard work fixing this issue.

—
Reply to this email directly or view it on GitHub
#48 (comment)
.

[matthewfarwell]

#129 has now been fixed, Could someone test it and see if it really does fix their issues please?

Thanks.

[leifwickland]

@matthewfarwell, Yes #129 is fixed for me. Thanks for your work.