Remove All PBF Defaults from Interface #3454
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ts are removable in proto3
… enums start at 0
kevinkreiser
requested review from
nilsnolde,
dgearhart,
dnesbitt61 and
purew
kevinkreiser
commented
Dec 11, 2021
Comment on lines
-93
to
-100
| http_request_t( | ||
| POST, | ||
| "/trace_attributes", | ||
| R"({"shape":[{"lat":37.8077440,"lon":-122.4197010},{"lat":37.8077440,"lon":-122.4197560},{"lat":37.8077450,"lon":-122.4198180}],"shape_match":"map_snap","best_paths":0,"costing":"pedestrian","directions_options":{"units":"miles"}})"), | ||
| http_request_t( | ||
| POST, | ||
| "/trace_attributes", | ||
| R"({"shape":[{"lat":37.8077440,"lon":-122.4197010},{"lat":37.8077440,"lon":-122.4197560},{"lat":37.8077450,"lon":-122.4198180}],"shape_match":"map_snap","best_paths":5,"costing":"pedestrian","directions_options":{"units":"miles"}})"), |
There was a problem hiding this comment.
instead of sending an error about asking for too many alternates in map matching i changed it to do what we do for regular routing which is to cap it. in the end this isnt a breaking change as the user wasnt getting back a usable response in the first place when asking for too few or too many routes.
There was a problem hiding this comment.
good as it's more inline with the general philosophy. I still think these implicit request parameter changes should be communicated in the response, e.g. #3099 . some day.. :)
genadz
pushed a commit
that referenced
this pull request
Jan 5, 2022
* store drive_on_left instead of right in trip edge so it can default to false * start ipa at 0 so that when we remove the default its stil the default * rename candidate ranking so its default can be false * default the map matching time in code rather than in pbf * handle search filter defaults in code * reorganize the low hanging fruit in options.proto so that some defaults are removable in proto3 * set roundabout_exits in code rather than in proto defaults * remove best_paths in favor of alternates * denoise is defaulted in code * default language in code * filter closures defaults in code * remove all of the trivial defaults. remove tracking boolean. make all enums start at 0 * changelog * forgot a trivial default * missed a config * have python test print out failure log when it fails. fix bad merge of static config
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is another step closer to proto3 for our protos. The main change here is to remove the use of
defaultwhich is not supported in proto3. There were two ways to do this.The first way is to simply remove the default value from the definition. This works if the default is what proto2/3 already default a scalar value to, from the manual:
For some of the values they defaulted to a certain enum value which was not the first enum value so i re-ordered those enums so that the default would be the first one. For some of the values that didnt default to
falsei simply change the name of the field to have the semantically opposite meaning so that a default offalsewould make sense.The second way is to move the defaulting inside of the code. This was required for those defaults which were not trivial like those above.
In addition to these things I did a couple more clean ups. The first one was I got rid of the
do_no_trackoption as no one ever used it and we didnt really adhere to it. The second is that i deprecated thebest_pathsoption in favor of the more descriptive and already existingalternatesoption. this parameter is still supported of course but it is translated intoalternatesat request time. I also made sure that all enums started with the first value at0which is required by proto3.NOTE: since deprecating
best_pathsi've also updated the config elements that configure the maximum values for that parameter. if you use an existing config the service will fail to start. the good news is you can just rename the config option (and subtract 1) and move on with your life. if you generate your configs before running the service you wont notice an issue.