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 ToPoint type class. #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aaabramov
Copy link
Collaborator

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

Main reason of adding this feature is to allow submitting
custom domain models to InfluxDB without constructing Point instance
every time.
@@ -12,12 +14,13 @@ class Database protected[influxdbclient]
with RetentionPolicyManagement
with DatabaseManagement
{
def write(point: Point,

def write[A](metric: A,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should not care about binary compatibility this time because artifact is being published under another group

* // ...
* }
* }}}
*/
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aren't these examples too verbose? @razem-io what do you think?

@aaabramov
Copy link
Collaborator Author

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
Copy link
Owner

razem-io commented Nov 22, 2019

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Owner

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.

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.

None yet

2 participants