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

Add id for CustomMessage (fix #495) #500

Merged
merged 2 commits into from
Dec 15, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion project/Dependencies.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import org.scalajs.sbtplugin.ScalaJSPlugin.autoImport._

object Dependencies {
val scalametaV = "2.1.2"
val metaconfigV = "0.5.3"
val metaconfigV = "0.5.4"
def semanticdbSbt = "0.4.0"
def dotty = "0.1.1-bin-20170530-f8f52cc-NIGHTLY"
def scala210 = "2.10.6"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package scalafix
package config

import metaconfig.{Conf, ConfError, ConfDecoder, Configured, Metaconfig}
import org.langmeta._
import scalafix.internal.config.MetaconfigPendingUpstream.XtensionConfScalafix

import scala.language.implicitConversions

class CustomMessage[T](
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that this is in our public API, can we add

type CustomMessage[T] = scalafix.config.CustomMessage[T]
val CustomMessage = scalafix.config.CustomMessage

in the scalafix package object? Rule authors should only need to import scalafix._; import scala.meta._ to get started.

val value: T,
val message: Option[String],
val id: Option[String])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add withValue/withMessage/withId helpers to make it more convenient to copy one field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You only read from the CustomMessage, I don't think this is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I agree.


object CustomMessage {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a apply(value: T) = new CustomMessage(value, None, None) constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ibid. you only read from the conf.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, nevermind!

def decoder[T](field: String)(
implicit ev: ConfDecoder[T]): ConfDecoder[CustomMessage[T]] =
ConfDecoder.instance[CustomMessage[T]] {
case obj: Conf.Obj => {
(obj.get[T](field) |@|
obj.getOption[String]("message") |@|
obj.getOption[String]("id")).map {
case ((value, message0), id) =>
val message =
message0.map(msg =>
if (msg.isMultiline) {
"\n" + msg.stripMargin
} else {
msg
})

new CustomMessage(value, message, id)
}
}
case els =>
ev.read(els).map(value => new CustomMessage(value, None, None))
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
package scalafix.internal.config
package scalafix
Copy link
Contributor

Choose a reason for hiding this comment

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

I generally avoid this pattern since it can easily introduce cycles in your codebase. import scalafix.config.CustomMessage is fine in our own code. However, I'm not so consistent myself 😸

package internal.config

import metaconfig.ConfDecoder
import MetaconfigPendingUpstream.XtensionConfScalafix
Expand All @@ -8,14 +9,13 @@ import scalafix.internal.util.SymbolOps
case class DisableConfig(symbols: List[CustomMessage[Symbol.Global]] = Nil) {
def allSymbols = symbols.map(_.value)

private val messageBySymbol: Map[String, String] =
private val messageBySymbol: Map[String, CustomMessage[Symbol.Global]] =
symbols
.flatMap(custom =>
custom.message.map(message =>
(SymbolOps.normalize(custom.value).syntax, message)))
.map(custom => (SymbolOps.normalize(custom.value).syntax, custom))
.toMap

def customMessage(symbol: Symbol.Global): Option[String] =
def customMessage(
symbol: Symbol.Global): Option[CustomMessage[Symbol.Global]] =
messageBySymbol.get(SymbolOps.normalize(symbol).syntax)

implicit val customMessageReader: ConfDecoder[CustomMessage[Symbol.Global]] =
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
package scalafix.internal.config
package scalafix
package internal.config

import metaconfig.{Conf, ConfError, ConfDecoder, Configured}
import org.langmeta._
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,18 @@ final case class Disable(index: SemanticdbIndex, config: DisableConfig)
case _ =>
"" -> pos
}
val message = config
.customMessage(symbol)
val custom = config.customMessage(symbol)

val message = custom
.flatMap(_.message)
.getOrElse(s"${signature.name} is disabled$details")

val id = custom
.flatMap(_.id)
.getOrElse(signature.name)

errorCategory
.copy(id = signature.name)
.copy(id = id)
.at(message, caret)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ final case class DisableSyntax(
while (matcher.find()) {
regexLintMessages +=
errorCategory
.copy(id = pattern)
.copy(id = regex.id.getOrElse(pattern))
.at(message, pos(matcher.start))
}
}
Expand Down
3 changes: 3 additions & 0 deletions scalafix-core/shared/src/main/scala/scalafix/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ package object scalafix {
@deprecated("Renamed to Rule", "0.5.0")
val Rewrite = rule.Rule

type CustomMessage[T] = scalafix.config.CustomMessage[T]
val CustomMessage = scalafix.config.CustomMessage

type SemanticRule = rule.SemanticRule
type Rule = rule.Rule
val Rule = rule.Rule
Expand Down
3 changes: 2 additions & 1 deletion scalafix-tests/input/src/main/scala/test/Disable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Disable.symbols = [
"test.Disable.D.disabledFunction"
{
symbol = "scala.Option.get"
id = "Option.get"
message =
"""|Option.get is the root of all evils
|
Expand Down Expand Up @@ -47,7 +48,7 @@ case object Disable {
def asInstanceOf: O = "test"
}
val yy = AA.asInstanceOf // OK, no errors
Option(1).get /* assert: Disable.get
Option(1).get /* assert: Disable.Option.get
^
Option.get is the root of all evils

Expand Down
3 changes: 2 additions & 1 deletion scalafix-tests/input/src/main/scala/test/DisableSyntax.scala
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ DisableSyntax.noSemicolons = true
DisableSyntax.noXml = true
DisableSyntax.regex = [
{
id = offensive
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot use pimp here because the assert would trigger the regex again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I wonder if we can avoid this in general by making sure the offensive part does not follow scalafix:ok/off. Since we have the start offset, we should be able to handle this. I opened #507 to track this.

Copy link
Contributor

Choose a reason for hiding this comment

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

"offensive" is better anyways.

pattern = "[P|p]imp"
message = "Please consider a less offensive word such as Extension"
}
Expand Down Expand Up @@ -39,7 +40,7 @@ case object DisableSyntax {
<a>xml</a> // assert: DisableSyntax.noXml
// assert: DisableSyntax.noTabs

implicit class StringPimp(value: String) { // assert: DisableSyntax.[P|p]imp
implicit class StringPimp(value: String) { // assert: DisableSyntax.offensive
def -(other: String): String = s"$value - $other"
}

Expand Down