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

messages: Parse enum types #40

Open
wants to merge 1 commit into
base: feature/13-add-data-types-for-messages
Choose a base branch
from

Conversation

robinkrahl
Copy link
Contributor

With this patch, values for enum fields are automatically parsed into the corresponding enum types when using the message structs. If a value cannot be parsed correctly, it is added to a invalid_fields field so that users can manually parse that data if they want to.

With this patch, values for enum fields are automatically parsed into
the corresponding enum types when using the message structs.  If a value
cannot be parsed correctly, it is added to a invalid_fields field so
that users can manually parse that data if they want to.
@robinkrahl
Copy link
Contributor Author

Updated:

  • Dropped support for Vec<_> to keep things simple for the time being.
  • Introduced the FromValue trait.

@stadelmanma
Copy link
Owner

I noticed you have some logic in there to convert the enum string/int value back into the "actual" type. Do you think we should keep the actual enum "instance" in the FitDataRecord instead of coercing them into strings/ints?

I'm not sure how exactly that would play out or if it could be implemented in a way that if someone was using them as an int/string the type conversion could happen automatically and not break existing code.

Relevant code in the profile/mod.rs file:

    // convert enum or rescale integer value into floating point
    if field_type.is_enum_type() {
        let val: i64 = value.try_into()?;
        if options.contains(&DecodeOption::ReturnNumericEnumValues) {
            Ok(Value::SInt64(val))
        } else if field_type.is_named_variant(val) {
            Ok(Value::String(get_field_variant_as_string(field_type, val)))
        } else {
            Ok(Value::SInt64(val))
        }
    } else {
        apply_scale_and_offset(value, scale, offset)
    }

@robinkrahl
Copy link
Contributor Author

Do you think we should keep the actual enum "instance" in the FitDataRecord instead of coercing them into strings/ints?

It would make sense, but it’s not a requirement for me.

I'm not sure how exactly that would play out or if it could be implemented in a way that if someone was using them as an int/string the type conversion could happen automatically and not break existing code.

I think it is hard not to break existing code with such a change. I would consider it sufficient (and easy) to provide a simple workaround like a conversion function.

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.

2 participants