[ISSUE #89]Add supports for additional fields in HR and Sleep. #92
Conversation
|
Hi, radar-schemas version 0.7.9 is now released (it might take a bit to be available on maven repo), please include that here so the CI can run the checks. Thanks. |
|
Ok, I have updated the radar-schemas version and Gradle runs successfully in my pc. |
There was a problem hiding this comment.
Mostly looks fine to me. Some minor comments.
Also, would be great if you can (if not done already) run and test against the actual fitibit API. Would also help you get set up with the dev workflow for any future work.
@blootsvoets, it would be great if you can review it too. Thanks.
|
|
||
| JsonNode restingHeartRateNode = value.get("restingHeartRate"); | ||
| if (restingHeartRateNode == null) { | ||
| logger.warn("Failed to get resting heart rate from {}, {}", request.getRequest().url(), root); |
There was a problem hiding this comment.
Can you make each of these log statement more informative? Right now all say the same thing.
| @@ -92,13 +95,14 @@ private List<RequestRoute> getRoutes(FitbitRestSourceConnectorConfig config) { | |||
| localRoutes.add(new FitbitIntradayHeartRateRoute(this, userRepository, avroData)); | |||
| localRoutes.add(new FitbitIntradayCaloriesRoute(this, userRepository, avroData)); | |||
| } | |||
| localRoutes.add(new FitbitRestingHeartRateRoute(this, userRepository, avroData)); | |||
There was a problem hiding this comment.
can you move this above the intraday ones so all the non-intraday are together?
There was a problem hiding this comment.
Ok, do I need to rename the FitbitRestingHeartRateRoute to FitbitIntradayRestingHeartRateRoute?
There was a problem hiding this comment.
No sorry, I think you misunderstood, I meant above the if statement (not inside). It is not intraday so the name is fine. Just wanted to move the statement together with others like
localRoutes.add(new FitbitSleepRoute(this, userRepository, avroData));
localRoutes.add(new FitbitTimeZoneRoute(this, userRepository, avroData));
localRoutes.add(new FitbitActivityLogRoute(this, userRepository, avroData));
localRoutes.add(new FitbitRestingHeartRateRoute(this, userRepository, avroData));
if (config.hasIntradayAccess()) {
localRoutes.add(new FitbitIntradayStepsRoute(this, userRepository, avroData));
localRoutes.add(new FitbitIntradayHeartRateRoute(this, userRepository, avroData));
localRoutes.add(new FitbitIntradayCaloriesRoute(this, userRepository, avroData));
}There was a problem hiding this comment.
Ok, I have updated codes according to your comments.
I have done the registration of Fitbit and run the docker according to readme in the very beginning, but it seems there is no data for my personal account. I will try to look into it in detail later. |
This PR is related to the issue #89
Changes made in the PR:
FitbitRestingHeartRateAvroConverter.javaFitbitRestingHeartRateRoute.javaFitbitSleepAvroConverter.javaFitbitRequestGenerator.javaFitbitRestSourceConnectorConfig.java