Developer fields
This PR is on top of "Apply linter suggestions for cleaner code".
This is a draft PR, I would like some feedback, but functionally it works for me: My fit files with developer fields are now correctly parsed. This addresses issue #41.
What changed:
- fields are now
HashMap<(u8, u8), Value>instead ofHashMap<u8, Value>: The first u8 is the dev_data_idx, or 255 if this i not a developer field (but a "normal" field). If I understand the fit protocol document correctly 255 is not a valid dev_data_idx. - We keep track of "field definitions" for developer fields, in a
HashMap<(u8, u8), DeveloperFieldDescription>calleddeveloper_fields. This is owned by the Decoder and when a field definition record is found, a new DeveloperFieldDescription is created from it, and added to the HashMap. The deserializer is passed a read only reference to this map: Inside is the information about theBaseTypethat is needed by the deserializer indata_message_fields_impl
Open questions:
- Could/Should we combine FitBaseType and BaseType? I would prefer to only use the auto-generated FitBaseType, and remove BaseType. Currently src/lib.rs:408 looks exceptionally ugly:
BaseType::from(FitBaseType::from(fit_base_type_id as &str).as_u8()), this could be nicer if BaseType and FitBaseTyper were the same thing. - The BaseType is currently converted to a string by the decoder, and then converted back to a enum entry from string. This is also not the nicest.
- Error handling: The fit file protocol states that e.g. developer fields must be defined before mentioned, but is it ok to use
.expectorpanic!if it doesn't follow this? Or should we handle errors more gracefully?
To address my open questions:
- I added a commit that removed
enum BaseTypeand replaces it with FitBaseType. It looks cleaner to me, and also less code: 3 files changed, 67 insertions(+), 125 deletions(-). Let me know if you agree with this. - Converting back the FitBaseType from String back to FitBaseType (in src/lib.rs:405) shouldn't be too big of a deal, as it occurs only once per developer field, so performance wise it is negligible.
- Still would like some feedback/checking if my error handling in this PR is ok :)
Unless I get any change requests, this is now good to go from my end - draft status revoked.
Great start on this! I went through an added some comments, for this change I'll need to run it over a separate corpus of test files that I've collected from other FIT file parsing repos.
I also merged your linter work as well.
All the comments have been addressed from my point of view, please take a look. Also, I have rebased the branch on the current master status (since the linter changes were merged).
If there is anything further I should do, please let me know. :)
Thank you for all your work on this and for fielding the in-depth discussion on how best to get this implemented!