influxdb-java
influxdb-java copied to clipboard
Request for annotation for null values on pojo fields of type String.
A @Default
annotation was requested to protect pojos coming back with null strings in #661. I interpreted it to mean something along the lines of what's in the unit test. Would appreciate feedback.
Codecov Report
Merging #693 into master will increase coverage by
0.04%
. The diff coverage is94.44%
.
@@ Coverage Diff @@
## master #693 +/- ##
============================================
+ Coverage 88.26% 88.31% +0.04%
Complexity 730 730
============================================
Files 69 69
Lines 2540 2558 +18
Branches 268 277 +9
============================================
+ Hits 2242 2259 +17
Misses 210 210
- Partials 88 89 +1
Impacted Files | Coverage Δ | Complexity Δ | |
---|---|---|---|
src/main/java/org/influxdb/dto/Point.java | 93.43% <94.44%> (+0.07%) |
53.00 <0.00> (ø) |
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 125a9ca...f54d76a. Read the comment docs.
Anyone with the ability to re-run jobs mind running the one that failed? All the others passed.
@BrentonPoke I restarted the tests, they now pass. I am not an expert in this library, I took a quick look at the new code while restarting the tests. It seems to me that
- the Default value always overrides any value of the field
- the new test is not testing the Default annotation, it checks that the set values are transferred to point
- the new Default annotation always sets a String value even for other supported field types (Double, Integer, Float, BigDecimal, Instant, Boolean), which would certainly be unexpected
- the relationship to #661 is not clear to me, it seems to me that it is already fixed by https://github.com/influxdata/influxdb-java/commit/6afc6ade28b073af2e53022316ada6a838b41815#diff-bd011207d3113c41cd8be4968d2321cb
@BrentonPoke I restarted the tests, they now pass. I am not an expert in this library, I took a quick look at the new code while restarting the tests. It seems to me that
- the Default value always overrides any value of the field
- the new test is not testing the Default annotation, it checks that the set values are transferred to point
- the new Default annotation always sets a String value even for other supported field types (Double, Integer, Float, BigDecimal, Instant, Boolean), which would certainly be unexpected
- the relationship to #661 is not clear to me, it seems to me that it is already fixed by 6afc6ad#diff-bd011207d3113c41cd8be4968d2321cb
Thanks for responding, I'm taking another look at this since I originally did it early in the morning, but the comment here is what was agreed upon as the desired change. I went off the end of this discussion that resolved the need-more-info label. For the test, I did struggle a bit in terms of trying to develop a test to check that the value is set to the default. I'm trying a Black Box testing approach, but this is my first time processing an annotation, so I'm doing this PR to learn how. I'm changing the processing of the annotation to only trigger on a String variable, but the reason I didn't include numerical values is that it wasn't part of the request agreed upon at the end of #661.
Ok, I think it's ready for another look, if anyone wants.