Skip to content
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

ExplicitResultTypes: skip local implicits #445

Merged
merged 6 commits into from
Nov 20, 2017

Conversation

erikwj
Copy link
Contributor

@erikwj erikwj commented Nov 17, 2017

fixes #34

Copy link
Contributor

@olafurpg olafurpg left a 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! A few comments, otherwise looks good 👍

nm: Name,
mods: Traversable[Mod],
body: Term,
printLocalTypes: Boolean)(implicit ev: Extract[D, Mod]): Boolean = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is printLocalTypes required? Seems config.skipLocalImplicits is only used as the argument

case None => false
}

isImplicit && (if (printLocalTypes) !isLocal else true) || {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to keep this boolean condition as simple as possible and delegate logic to local defs. We can move this if/else into isLocal

@@ -125,16 +134,16 @@ case class ExplicitResultTypes(
}

ctx.tree.collect {
case t @ Defn.Val(mods, _, None, body)
if isRuleCandidate(t, mods, body) =>
case t @ Defn.Val(mods, Seq(Pat.Var(name)), None, body)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, scalameta trees only use List, conventionally we would pattern match this like Pat.Var(name) :: Nil

@olafurpg olafurpg changed the title 34 local implicit val ExplicitResultTypes: skip local implicits Nov 17, 2017
Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thank you @erikwj !

### Configuration

```scala
ExplicitResultTypes.memberKind = [Val, Def, Var]
ExplicitResultTypes.memberVisibility = [Public, Protected]
ExplicitResultTypes.memberKind = [Val, Def, Var] // empty by default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

@olafurpg olafurpg merged commit 994a605 into scalacenter:master Nov 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExplicitImplicit: allow to omit types for local implicit vals
2 participants