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

allow to figure out, whether the Point.Builder has any fields #434

Merged
merged 1 commit into from
Mar 26, 2018

Conversation

kub
Copy link
Contributor

@kub kub commented Mar 23, 2018

since the build mehtod contains validation for fields emptiness, there should be also way, how to figure out, whether the Builder contains any fields (to prevent the build method from throwing an exception)

@codecov-io
Copy link

codecov-io commented Mar 23, 2018

Codecov Report

Merging #434 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #434      +/-   ##
============================================
+ Coverage     85.93%   85.94%   +0.01%     
  Complexity      292      292              
============================================
  Files            20       20              
  Lines          1258     1259       +1     
  Branches        131      132       +1     
============================================
+ Hits           1081     1082       +1     
  Misses          116      116              
  Partials         61       61
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/influxdb/dto/Point.java 82.92% <100%> (+0.1%) 31 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77b5f37...997992e. Read the comment docs.

@majst01
Copy link
Collaborator

majst01 commented Mar 23, 2018

What exact problem are you trying to solve, can you show it in a testcase ? Dont see the point actually.

@kub
Copy link
Contributor Author

kub commented Mar 24, 2018

I'll try to explain my problem more into the detail, hope it helps:
as the Point is constructed by the Point.Builder, Point.Builder#build() can be called in different part of the application (different method), than Point.Builder#addField. In such case, there is no way how to avoid IllegalArgumentException, that is thrown from here in case, that no fields were actually added (e.g. because data of these fields was null).

It's also convenient for other cases. Imagine e.g. this one:

void reportValues(String value1, String value2, String value3) {
    def builder = Point.measurement(measurement).tag(tags).time(time)

    if(value1 != null) {
        builder.addField(TIMESERIES_1, value1)
    }

    if(value2 != null) {
        builder.addField(TIMESERIES_2, value2)
    }

    if(value3 != null) {
        builder.addField(TIMESERIES_3, value3)
    }

    builder.build() // throws IllegalArgumentException, if all values are null

}

... again - there should be a way, how to 'ask' the builder, whether it is ok, to call the build method or whether it will fail with the IllegalArgumentException.

@majst01
Copy link
Collaborator

majst01 commented Mar 24, 2018

Ok understood, but then you need to add a testcase and a addition to the usage description in the README.md to you MR.

since the build mehtod contains validation for fields emptiness, there should be also way, how to figure out, whether the Builder contains any fields (to prevent the build method from throwing an exception)
@kub
Copy link
Contributor Author

kub commented Mar 25, 2018

I've added unit test

I'm not sure about README.md - do you really think, that this small detail should be really added there? Tried to read through it and honestly - I'm not sure where to put it. Also seems to me, that README.md should not go into that level of detail (just for sake of readability) - what do you think?

@majst01 majst01 merged commit b25c197 into influxdata:master Mar 26, 2018
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

3 participants