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

Standardize how we talk about OSRM #262

Open
ianthetechie opened this issue Sep 23, 2024 · 2 comments
Open

Standardize how we talk about OSRM #262

ianthetechie opened this issue Sep 23, 2024 · 2 comments
Labels
chore It has to be done sometime... documentation Improvements or additions to documentation good first issue Good for newcomers

Comments

@ianthetechie
Copy link
Contributor

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.

@ianthetechie ianthetechie added documentation Improvements or additions to documentation good first issue Good for newcomers chore It has to be done sometime... labels Sep 23, 2024
@SGI-CAPP-AT2
Copy link

SGI-CAPP-AT2 commented Oct 24, 2024

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
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore It has to be done sometime... documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
Status: No status
Development

No branches or pull requests

2 participants