search

stadiamaps / ferrostar

A FOSS navigation SDK built from the ground up for the future
https://stadiamaps.github.io/ferrostar/
Other
168 stars 19 forks source link

Standardize how we talk about OSRM #262

Open ianthetechie opened 1 month ago

ianthetechie commented 1 month ago

Our current nomenclature can be a bit confusing to newcomers because we use the term "OSRM" rather liberally when not intending to refer exclusively to OSRM routing. In particular our OSRM response parser and models are not actually specific to any vendor, and are more of a loose union of all the extensions that we're using, modeled as optional parameters.

We had a discussion about this on Slack and decided on "extended OSRM" as a general term where we are intentionally broad and expect to support either OSRM itself or something that "looks like an OSRM response."

Turned into a statement, here's what we'll refactor to:

createExtendedOsrmResponseParser is a function which creates an ExtendedOsrmResponseParser. This object implements the RouteResponseParser trait and is capable of handling both the “original” OSRM format and common extensions to it, including voice and banner instructions in the format popularized by Mapbox.

SGI-CAPP-AT2 commented 1 week ago

Hey @ianthetechie , I would like to contribute on this issue, but as I am new to this repo a quick introduction may help me get started.

ianthetechie commented 4 days ago

Sorry for the delay getting back to you @SGI-CAPP-AT2, but have you checked out the documentation? That contains everything from an architecture overview to contribution best practices.

On this specific task, I think the easiest approach might be to do a repo-wide search for "OSRM" and then use your judgement on refactoring (it's easier using dev tools like Android Studio, Xcode, and RustRover which have proper refactoring tools). Certain usages definitely do apply specifically to OSRM, but others (particularly publicly exposed function names) will probably make more sense with an "extended" prefix.

Here's an example: https://github.com/stadiamaps/ferrostar/blob/main/common/ferrostar/src/routing_adapters/osrm/mod.rs. This file probably needs a header comment update to unify around the new terminology, I'd change the OsrmResponseParser to ExtendedOsrmResponseParser, I'd change the function from_osrm to from_extended_osrm, etc. However, I probably wouldn't change the fixture names in the unit tests (not going to confuse anyone with those).

If anything is unclear, feel free to leave a comment / review note on your PR so we can take a look.

Hope this helps!