-
Notifications
You must be signed in to change notification settings - Fork 185
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
Read ExplicitResultTypes key instead of explicitReturnTypes #354
Read ExplicitResultTypes key instead of explicitReturnTypes #354
Conversation
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.
Thank you for this contribution @insdami ! One small comment, otherwise looks good :)
@@ -33,7 +33,7 @@ case class ExplicitResultTypes( | |||
this(index, ExplicitResultTypesConfig.default) | |||
override def init(config: Conf): Configured[Rule] = | |||
config | |||
.getOrElse("explicitReturnTypes")(ExplicitResultTypesConfig.default) | |||
.getOrElse("explicitResultTypes")(ExplicitResultTypesConfig.default) |
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.
This will break people's existing configuration, I think we will have to support both names throughout 0.5.x. We currently have no means to emit a deprecation warning here, so no worries about that.
There's no high-level api on Conf
to fallback on a different key, but a simple workaround is to check if the resulting setting is still default and then try the fallback name.
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.
Oh, and one more comment, see https://github.com/scalacenter/scalafix/pull/355/files#r139296465 what do you think about making it "ExplicitResultTypes" with capital E in the beginning?
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.
@olafurpg cool! I just pushed a new commit. To support the deprecated config I've used extraNames
in Config#getOrElse
. Let me know if there is something else that could be improved.
f92c5b4
to
e5216b6
Compare
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.
LGTM, good work @insdami 😄.
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.
LGTM 👍 thank you!
Small contribution from Dublin Scala Spree on #335