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

Convert MaxSpeed to consistent JSON format (a float of meters per second) #272

Closed
Archdoog opened this issue Sep 26, 2024 · 5 comments
Closed
Labels
android core Related to the Rust core iOS

Comments

@Archdoog
Copy link
Collaborator

Archdoog commented Sep 26, 2024

The MaxSpeed json object has two formats:

  1. {"unknown": true} - The speed limit is unknown
  2. {"speed":30.2,"unit":"km/h"} - The speed is unknown and potentially the localized unit (depending on API).

This is an annoying object to deserialize on some platforms where json serialization requires typed references often requiring a custom decode handler. Because of this, it's probably best that we fix this once in rust to standardize the type definition. Suggestion is probably:

/// Speed limit in meters per second. None if unknown.
speed_limit: Option<f64>
// TODO: This is a bonus as the platforms will be able to convert to KPH, MPH, etc.
// Some apps may prefer localized units from a route engine that supports it, device locale,
// or even manual user preference.
local_speed_limit_unit: Option<String> 

This is a much easier thing to deserialize as it always has the same type definition on a platform.

@ianthetechie
Copy link
Contributor

Why can we just do enum with two variants where one variant has two required fields (and the unit is an enum of course) and the other has none?

enum SpeedLimit {
    Unlimited,
    Unknown,
    Posted {
        speed: f32,
        unit: SpeedUnit
    }
}

And for platform code that wants it in a standard unit, we can have methods on the enum impl. We can't expose these in UniFFI directly but we can expose top level functions for convenience.

This should keep the data model lossless while letting us centralize conversion logic where it's needed for some purpose other than display.

@Archdoog
Copy link
Collaborator Author

We can absolutely just provide the custom deserializers on each platform required to handle the untouched MaxSpeed from the backend. I'm all for that, as long as developers can just include a MaxSpeed model as a child in any custom attributes model definition.

On Swift we can provide a custom MaxSpeed: Codable with its specialized Decoder and encode methods assuming https://www.swiftbysundell.com/articles/codable-synthesis-for-swift-enums/ doesn't provide any useful automatic solutions. Will have to research the same on Kotlin.

My main goal is avoiding friction when you define your slightly custom Attributes model on whatever platform. Enum with value patterns are not commonly very nice with serialization tools. Rust serde is the exception where it works perfectly out of the box.

@ianthetechie
Copy link
Contributor

Yeah, understood. I think we can do that with the UniFFI magic in the toml file we were chatting about earlier. I'd be happy to put in some work next week to make that happen if you can work up some rough code that you'd like to see compilable. (i.e. draft of something that probably doesn't work but that you'd like to just magically work)

I think we're getting close on this :D

@ianthetechie
Copy link
Contributor

I also see you had implemented some of the ideas already in your PR now :P Still working through that...

@Archdoog
Copy link
Collaborator Author

Think this can be dropped as a result of #287. Android may be a little harder, but the benefits of this system seem to be solid enough as demonstrated on the iOS ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android core Related to the Rust core iOS
Projects
Status: Done
Development

No branches or pull requests

2 participants