`fromBooleanCheck` has dangerous signature with by-name Parameter in implicit about metrics-scala HOT 7 CLOSED

Wow, that's a pretty impressive finding. Thanks for that!

Also thanks for the 2 suggestions. I'll have to think about which one I prefer. @skullxbones, do you have a preference?

from metrics-scala.

I learned something today, thank you! I didn't realize that by value and implicit interacted this way.

Let me write up that failing test case. Probably making the signature Unit => A is the way to go since it's less magical and more explicit.

from metrics-scala.

This issue is related: https://issues.scala-lang.org/browse/SI-3237.

from metrics-scala.

FWIW, I think changing the signature of healthCheck to specify by-name semantics is the obvious minimal change. Otherwise, you wouldn't expect this to work either:

  @Test def explicit(): Unit = {
    var count = 0
    var checker = false
    val res = healthCheck("bool test","nope") {
      count += 1 
      new HealthCheckMagnet {
        def apply(unhealthyMessage: String) = () => if (checker) "OK" else unhealthyMessage
      } 
    } 
    assertEquals("nope", res())
    checker = true
    assertEquals("OK", res())
    assertEquals(2, count)
  }

Here is the minimization I was working with.

import language._

package object health {
  type HealthCheck = () => String

  def healthCheck(name: String, unhealthyMessage: String = "Health check failed")(checker: =>HealthCheckMagnet): HealthCheck = {
    () => checker(unhealthyMessage)()
  } 
}     

package health {

  /*sealed*/ trait HealthCheckMagnet {
    def apply(unhealthyMessage: String): HealthCheck
  } 

  object HealthCheckMagnet {

    /** Magnet for checkers returning a [[scala.Boolean]] (possibly implicitly converted).  */
    implicit def fromBooleanCheck[A <% Boolean](checker: => A) = new HealthCheckMagnet {
      def apply(unhealthyMessage: String) = () => if (checker) "OK" else unhealthyMessage
    }   
  }   
}   

with test

  @Test def bools(): Unit = {
    var count = 0
    val res = healthCheck("bool test","nope") {
      count += 1
      false
    }
    assertEquals("nope", res())
    assertEquals("nope", res()) 
    assertEquals(2, count)
  }

The other difference may be that healthCheck is not invoking checker immediately.

Frankly, I'm confused by the checkers and checks, and I lost my scorecard. And am I the only one who keeps thinking "chick magnet"?

from metrics-scala.

I am rather to old to be a chick magnet, but it sound great nonetheless!

Thanks for the idea of making the parameter by-name also. Your prototype should be very easy to integrate. Am I right that this will break binary compatibility (but keep source compatibility)?

from metrics-scala.

There is an interesting twist you didn't consider. The (java) class HealthCheck comes from metrics-core and we can't just redefine that.

from metrics-scala.

Anyway, just declaring the checker as by name was sufficient. Thanks!

from metrics-scala.