Refactored and simplified file structure and added data models for in… #8
Conversation
…tervals, heart rate, and full results (retrieved from a GET call on C2's api rather than from their webhook payload)
| @@ -0,0 +1,11 @@ | |||
| part of 'index.dart'; | |||
|
|
|||
| class C2DetailedResult { | |||
There was a problem hiding this comment.
Is there a reason this class exists as a separate thing from C2WebhookResult? it seems to borrow similar comments from that class and serve the same intended function.
There was a problem hiding this comment.
the webhook almost gives you all the data you need (except the number of intervals) and making a separate REST call (which may not be fully propagated across C2's network - fun fact) to the c2logbook API to retrieve this extra data is where this comes from
| part of 'index.dart'; | ||
|
|
||
| @freezed | ||
| class C2FullResults with _$C2FullResults { |
There was a problem hiding this comment.
Does this class provide something separately from the C2Results class? or is it largely just a copy to make changes less destructive/easy to undo/allow more rapid iteration?
There was a problem hiding this comment.
Does this have something to do with differences between the result data from the webhook vs a GET call?
| factory C2FullResults({ | ||
| @JsonKey(name: 'id') @Default(0) int id, | ||
| @JsonKey(name: 'user_id') @Default(0) int userId, | ||
| @JsonKey(name: 'date') @TimestampConverter() required DateTime date, |
There was a problem hiding this comment.
I notice in all the refactoring the field name for this got renamed from endDate (which i find to be more specific description of its purpose). I assume it was changed back to try and get the json keys to work automatically, but figured id check if theres a reason this cant be changed back now that the @JsonKey annotation is back.
There was a problem hiding this comment.
this is ok to change but make crewlab a migration guide for name changes
| @JsonKey(name: 'rest_time') @DecimalIntConverter.tenths() double? restTime, | ||
| @JsonKey(name: 'stroke_rate') int? strokeRate, | ||
| @JsonKey(name: 'heart_rate') @Default(null) C2HeartRate? heartRate, | ||
| @JsonKey(name: 'workout') @Default(null) C2Workout? workout, |
There was a problem hiding this comment.
was there an issue with the json keys for fields that dont have multiword names (like workout) where they werent getting recognized without making it explicit? or were these just added back for code cleanliness/formatting consistency?
I noticed when testing that if the key and the field name are the same, the @JsonKey isnt necessary to make explicit
There was a problem hiding this comment.
some fields werent making it in. adding them all fixed it
There was a problem hiding this comment.
is this config something thats built into dart to help automate the generation of code during the build? or is explicitly running the code gen still necessary?
| @Default('') String email, | ||
| @Default('') String country, | ||
| @JsonKey(name: 'profile_image') String? profileImage, |
There was a problem hiding this comment.
did i get a bunch of the JSON keys in this file wrong? or were you able to find a way to automate them based on the field name in dart?
|
Seems like the CI is failing because of some dart formatting (i dont have access to the branch that this PR is from in order to commit the fix to that). definitely agree with the overall approach of this PR (especially love how all the generated code is combined into one file). Havent had a chance to run the unit tests yet. Are there any new testcases that may help ensure that future changes dont break crewLAB's usecase? Maybe some extra action items:
|
…tervals, heart rate, and full results (retrieved from a GET call on C2's api rather than from their webhook payload)