Skip to content

openlr: Explicitly check for linear reference option for Valhalla serialization#2458

Merged
mookerji merged 4 commits intomasterfrom
mookerji-valhalla-no-default-linear-references
Jul 10, 2020
Merged

openlr: Explicitly check for linear reference option for Valhalla serialization#2458
mookerji merged 4 commits intomasterfrom
mookerji-valhalla-no-default-linear-references

Conversation

@mookerji
Copy link
Copy Markdown
Contributor

@mookerji mookerji commented Jul 6, 2020

In a prior PR (link below) we added OpenLR encoding for trace_route edges but apparently did not
check for the API option, defaulting to adding OpenLR linear references to the response. This fixes
this issue and adds the options checking to the library function filling out references.

Follow up to:
#2424

/cc @kevinkreiser

@mookerji mookerji requested a review from kevinkreiser July 6, 2020 23:58
@mookerji mookerji self-assigned this Jul 6, 2020
@mookerji mookerji force-pushed the mookerji-valhalla-no-default-linear-references branch from 017f6ea to 1e5aea8 Compare July 9, 2020 02:04
Comment thread src/tyr/serializers.cc Outdated
if (!linear_reference) {
return;
}
if (options.costing() != Costing::auto_) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should remove this if, I know that frc will potentially be mostly 7 but that's ok

Comment thread src/tyr/route_serializer_valhalla.cc Outdated
kevinkreiser
kevinkreiser previously approved these changes Jul 9, 2020
Copy link
Copy Markdown
Member

@kevinkreiser kevinkreiser left a comment

Choose a reason for hiding this comment

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

Other than 2 small comments looks good

Buro Mookerji added 3 commits July 9, 2020 11:32
…ialization.

In a prior PR (link below) we added OpenLR encoding for trace_route edges but apparently did not
check for the API option, defaulting to adding OpenLR linear references to the response. This fixes
this issue and adds the options checking to the library function filling out references.

#2424

/cc @kevinkreiser
@mookerji mookerji force-pushed the mookerji-valhalla-no-default-linear-references branch from 1e5aea8 to fbbee15 Compare July 9, 2020 19:04
@mookerji mookerji merged commit 75cb099 into master Jul 10, 2020
yuzheyan added a commit that referenced this pull request Jul 13, 2020
* 'master' of github.com:valhalla/valhalla:
  Hide robin hood (#2455)
  openlr: Explicitly check for linear reference option for Valhalla serialization (#2458)
  docs: add luajit to macOS installation (#2461)
  Insert Shape Points at TrafficSpeed Breakpoints (#2451)
@nilsnolde nilsnolde deleted the mookerji-valhalla-no-default-linear-references branch February 24, 2024 14:59
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.

2 participants