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

#371 Support dynamic measurement name in InfluxDBResultMapper #423

Merged
merged 2 commits into from
Apr 28, 2018
Merged

#371 Support dynamic measurement name in InfluxDBResultMapper #423

merged 2 commits into from
Apr 28, 2018

Conversation

ikercrg
Copy link

@ikercrg ikercrg commented Mar 1, 2018

Checkstyle not added. Please review it. Added just one single method that supports a dynamic measurement name.

@fmachado fmachado self-requested a review March 1, 2018 13:08
Copy link
Contributor

@fmachado fmachado left a comment

Choose a reason for hiding this comment

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

Please update the unit tests with tests that could cover your changes and address the issues mentioned in my comments. Thanks!

throwExceptionIfResultWithError(queryResult);
cacheMeasurementClass(clazz);

List<T> result = new LinkedList<T>();
String measurementName = getMeasurementName(clazz);

if (measurementName == null || measurementName.isEmpty())
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move these verification to the top of the method. IMO Objects.requireNonNull(measurementName, "measurementName"); would be enough and the code should be resilient against an empty string.

String measurementName = getMeasurementName(clazz);

if (measurementName == null || measurementName.isEmpty())
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This block { } is not compliant with the existing checkstyle.xml that you can find at the root of the project. If you have considered my previous argument as valid (Object.requireNonNull as enough), just remove this block.

parseSeriesAs(series, clazz, result);
});
});
.filter(internalResult -> Objects.nonNull(internalResult) && Objects.nonNull(internalResult.getSeries()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, do not change the indentation of lines that are not direct related with your changes. Unfortunately you will have to revert these changes.

@codecov-io
Copy link

Codecov Report

Merging #423 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #423      +/-   ##
============================================
+ Coverage     84.86%   84.89%   +0.02%     
- Complexity      270      271       +1     
============================================
  Files            19       19              
  Lines          1183     1185       +2     
  Branches        119      119              
============================================
+ Hits           1004     1006       +2     
  Misses          118      118              
  Partials         61       61
Impacted Files Coverage Δ Complexity Δ
...n/java/org/influxdb/impl/InfluxDBResultMapper.java 87.78% <100%> (+0.18%) 46 <2> (+1) ⬆️

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 be3b8b6...a00cd83. Read the comment docs.

@fmachado fmachado merged commit d374d6d into influxdata:master Apr 28, 2018
@lxhoan lxhoan added this to the 2.11 milestone May 14, 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

4 participants