-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Deprecated Procedure Syntax and Specify Return Types #1420
Conversation
- use Scalafix ProcedureSyntax and ExplicitResultTypes rules DAFFODIL-2152
@@ -71,15 +71,15 @@ import org.xml.sax.helpers.DefaultHandler | |||
object InfosetType extends Enumeration { | |||
type Type = Value | |||
|
|||
val EXI = Value("exi") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SpecifyReturnType
fix seems to add a lot of extra verbosity. I assume this is just a style change and not something deprecated by scala 2.13 or 3.x? There may be some places where having an explicit return type might be useful, but I'm not sure we should require it everywhere if future scala versions don't require it.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the old PR, I assumed return types were required in 2.13. I'll do more research and see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it looks like it's recommended but not strictly enforced from https://docs.scala-lang.org/scala3/guides/migration/incompatibility-table.html#type-inference
It is always good practice to write the result types of all public values and methods explicitly. It prevents the public API of your library from changing with the Scala version, because of different inferred types. This can be done prior to the Scala 3 migration by using the [ExplicitResultTypes](https://scalacenter.github.io/scalafix/docs/rules/ExplicitResultTypes.html) rule in Scalafix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the Scala 3 book: https://docs.scala-lang.org/scala3/book/methods-most.html
It says
Declaring the method return type is optional
and
It is recommended to annotate publicly visible methods with their return type. Declaring the return type can make it easier to understand it when you look at it months or years later, or when you look at another person’s code.
So I don't think it's required, but is just good practice. But we have so many public members that I think it would make things too verbose. We may should be better about marking things as private, but that's a whole other effort, and it's probably not worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I can close the PR and submit one with just the procedure syntax
DAFFODIL-2152