razem-io / scala-influxdb-client

Asynchronous InfluxDB client for Scala
MIT License
5 stars 3 forks source link

Add `ToPoint` type class. #2

Open aaabramov opened 4 years ago

aaabramov commented 4 years ago

Main reason of adding this feature is to allow submitting custom domain models to InfluxDB without constructing Point instance every time.

aaabramov commented 4 years ago

Also, @razem-io, what do you think about adding same semantics for bulkWrite? I mean we can introduce implicit ToPoint for Seq of Points.

razem-io commented 4 years ago

Hey @aaabramov , thank you for the PR. The idea is good but why should we implement it on library level? Wouldn't implicit conversions and classes not on library level be a more lightweight approach?

I made a quick example, which also compiles and does not change the code of the library:

  test("A point can be written using ToPoint type class") {
    await(database.write(Metric(123, "tag_value")))
    val result = await(database.query("SELECT * FROM test_measurement WHERE tag_key='tag_value'"))
    assert(result.series.length == 1)
  }

  test("A point can be written using ToPoint syntax") {
    await(database.write(Metric(123, "tag_value").toPoint))
    val result = await(database.query("SELECT * FROM test_measurement WHERE tag_key='tag_value'"))
    assert(result.series.length == 1)
  }

  test("A point can be written using implicit conversion") {
    await(database.write(Metric(123, "tag_value")))
    val result = await(database.query("SELECT * FROM test_measurement WHERE tag_key='tag_value'"))
    assert(result.series.length == 1)
  }

  case class Metric(value: Int, tag: String)

  object Metric {
    import scala.language.implicitConversions

    implicit def metric2Point(m: Metric): Point =
      Point("test_measurement")
        .addField("value", m.value)
        .addTag("tag_key", m.tag)

    implicit class RichMetric(x: Metric) {
      def toPoint: Point = {
        metric2Point(x)
      }
    }
  }
aaabramov commented 4 years ago

This is exactly the approach I am currently using in my projects. If you think it should not be on library level so we can decline this one. As I said previously:

This PR is only a Proof of concept and is a subject to discuss. If this feature will be accepted I will add proper documentation.

aaabramov commented 4 years ago

Moreover, I prefer using type classes rather than implicit conversions because they are way more flexible and clear. See: https://stackoverflow.com/a/8535107/5091346

razem-io commented 4 years ago

I also thought about it, let's just do it. It won't change the current api and just makes things easier. Support for bulk sounds good.