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

Extend DisableSyntax Rule to Regex #436

Closed
MasseGuillaume opened this issue Nov 16, 2017 · 7 comments
Closed

Extend DisableSyntax Rule to Regex #436

MasseGuillaume opened this issue Nov 16, 2017 · 7 comments

Comments

@MasseGuillaume
Copy link
Contributor

The apache spark scalastyle config goes a long way using only syntax. For example, it can report usage of
Await.ready and provide a custom message:

Are you sure that you want to use Await.ready? In most cases, you should use ThreadUtils.awaitReady instead.
If you must use Await.ready, wrap the code block with
// scalastyle:off awaitready
Await.ready(...)
// scalastyle:on awaitready

https://github.com/apache/spark/blob/master/scalastyle-config.xml#L206-L215

@MasseGuillaume
Copy link
Contributor Author

@olafurpg I think it would make sense to extend the idea to custom message to all Rule.

Disable (Semantic):

rules = Disable
Disable.symbols = [
  "Await.ready"
]

to

rules = Disable
Disable.symbols = [
  {
    symbol: "Await.ready"
    message: 
     """
Are you sure that you want to use Await.ready? In most cases, you should use ThreadUtils.awaitReady instead.
If you must use Await.ready, wrap the code block with
// scalastyle:off awaitready
Await.ready(...)
// scalastyle:on awaitready
     """
  }
]

nb: no strip margin for typesafe config: lightbend/config#391

@olafurpg
Copy link
Contributor

I agree @MasseGuillaume we should make it possible to attach a custom message to any disable rule, including symbols. Adding support for this in the configuration should be possible with a generic helper like

case class CustomMessage[T](value: T, message: Option[String])
object CustomMessage {
  implicit 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.get[String]("message")).map {
          case (value, message) => CustomMessage(value, Some(message))
        }
      case els =>
        ev.read(els).map(value => CustomMessage(value, None))
    }
}
// Use it like this
val regexpDecoder =
  ScalafixMetaconfigReaders.CustomMessage.decoder[Regex]("regexp")

@MasseGuillaume
Copy link
Contributor Author

@olafurpg hum, so we turn every configuration fields into CustomMessage[T] ?

@olafurpg
Copy link
Contributor

Do you have an alternative suggestion?

@MasseGuillaume
Copy link
Contributor Author

MasseGuillaume commented Nov 17, 2017

Do you have an alternative suggestion?

Nope, I just want to confirm we want to do this before doing any major refactoring like this.

Also, testkit should allow multiline asserts

Await.result(longComputation)// assert: Disable.Await.result

to

Await.result(longComputation)
/*
^ DisableSyntax.Await.result

Are you sure that you want to use Await.ready? In most cases, you should use ThreadUtils.awaitReady instead.
If you must use Await.ready, wrap the code block with
// scalafix:off DisableSyntax.Await.result
Await.ready(...)
// scalafix:on DisableSyntax.Await.result
*/

@olafurpg
Copy link
Contributor

Nope, I just want to confirm we want to do this before doing any major refactoring like this.

I think this would be a nice addition, since it's not very helpful when scalafix reports "X is disabled" with no context. Usually there is a story at every company why X is not allowed.

Also, testkit should allow multiline asserts

The testkit assertions are only against the LintCategory.id, not the reported message.

@olafurpg
Copy link
Contributor

Fixed in #494 @MasseGuillaume ?

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

No branches or pull requests

2 participants