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

Request for annotation for null values on pojo fields of type String. #693

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

Conversation

BrentonPoke
Copy link

@BrentonPoke BrentonPoke commented Aug 16, 2020

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.

@BrentonPoke BrentonPoke reopened this Aug 16, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 16, 2020

Codecov Report

Merging #693 into master will increase coverage by 0.04%.
The diff coverage is 94.44%.

Impacted file tree graph

@@             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.

@BrentonPoke BrentonPoke changed the title Fix for #661 Requesting annotation for null values on pojo fields of type String. Aug 16, 2020
@BrentonPoke BrentonPoke changed the title Requesting annotation for null values on pojo fields of type String. Request for annotation for null values on pojo fields of type String. Aug 16, 2020
@BrentonPoke
Copy link
Author

Anyone with the ability to re-run jobs mind running the one that failed? All the others passed.

@sranka
Copy link

sranka commented Aug 20, 2020

@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 null/empty values mapped to 0 for a double ? #661 is not clear to me, it seems to me that it is already fixed by 6afc6ad#diff-bd011207d3113c41cd8be4968d2321cb

@BrentonPoke
Copy link
Author

BrentonPoke commented Aug 20, 2020

@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 null/empty values mapped to 0 for a double ? #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.

@BrentonPoke
Copy link
Author

Ok, I think it's ready for another look, if anyone wants.

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.

3 participants