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

Refactor Costing and PathEdge Protos #3506

Merged
merged 8 commits into from
Jan 12, 2022
Merged

Refactor Costing and PathEdge Protos #3506

merged 8 commits into from
Jan 12, 2022

Conversation

kevinkreiser
Copy link
Member

@kevinkreiser kevinkreiser commented Jan 10, 2022

in #3464 we added support for interacting with the service via protobuf. at the time we noted several items for future work. this pr closes out the "refactor" item.

refactor the options message (separate PR): the request options message was incepted before we intended to use it as a request format. Because of that it's become pretty crufty and we should take a hard look at it before taking this feature out of beta.

the specific refactoring that was done here is all contained to the "request" (ie options object). inside of the options object there are several output items, which is a bit strange as this is the request object we use to start the service interaction going. there are mainly two places where we actually store output which we would not expect the user to provide. those two places are the ones listed in the title, the costing object and the path edges that loki finds. let me summarize the changes.

PathEdge
Loki takes your input locations and finds candidate edges in the graph that we can use in the algorithms. We currently store this information in the Location object which is also where the requester specifies the lat lng etc. In addition to the path edges there are a few more bits of output info about where the location correlated to the graph. Instead of having these all directly in the location object I nested them into a new object called correlation and marked it so that its obvious that its output. I could have done something more radical and moved it completely out of the options object but this didnt feel necessary.

Costing
Currently we have a Costing enum which must be specified as well as CostingOptions which are optional. At the moment costing options has both inputs and outputs. The refactor puts all of the proper costing options into their own object and nests that inside of a costing object. In this way we can have the higher level non optional bits of a costing object as well as the output bits separate from the actual user supplied options. ive also changed the naming a bit so that the enum is called Costing::Type and costing options are now Costing::Options.

Thats it. There is no functional change to the code in anyway its all moves and renames to hopefully make the api a bit easier to work with in pbf mode.

@kevinkreiser kevinkreiser requested review from a team and removed request for a team January 12, 2022 15:25
Copy link
Member

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just looked at the structural changes of protobuf and looks reasonable to me!

Copy link
Member

@dgearhart dgearhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢

@kevinkreiser kevinkreiser merged commit 9517a7b into master Jan 12, 2022
@kevinkreiser kevinkreiser deleted the kk_pbf_refactor branch January 12, 2022 23:14
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.

3 participants