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

Return name and ref response fields separately #2857

Merged
merged 1 commit into from
Sep 8, 2016
Merged

Return name and ref response fields separately #2857

merged 1 commit into from
Sep 8, 2016

Conversation

karenzshea
Copy link
Contributor

@karenzshea karenzshea commented Sep 5, 2016

Issue

Ref #2704
Splits name and ref into separate fields in the response object.

Tasklist

  • add regression / cucumber cases (see docs/testing.md)
  • review
  • adjust for for comments

Requirements / Relations

Enables #2744 and #2845

cc @daniel-j-h @MoKob @emiltin

@daniel-j-h
Copy link
Member

@emiltin this will finally provide separate name, ref, destination fields in the response when it lands, no longer having to parse the weird name (ref) construct we're currently having.

Which also means your parsing code will probably have to change. Just keeping you in the loop here.

@daniel-j-h
Copy link
Member

daniel-j-h commented Sep 5, 2016

Tasks

  • add a ref property to the ExtractionWay type
  • register this new property for Lua/Luabind in the LuaScriptingEnvironment
  • rewrite profiles to separately write result.name result.ref result.destinations; no more hacks
  • in extractor_callbacks.hpp adapt the string deduplication map
  • in extractor_callbacks..cpp deduplicate and add ref to external_memory datastructure
  • add a ref attribute to the RouteStep type representing a step in the response
  • add function for returning the ref based on name id to datafacades (base, internal, shared, mock)
  • in assemble_steps.hpp call datafacade function and add the result to the RouteStep object
  • in json_factory.cpp serialize the RouteStep's new attribute to the json::object
  • adapt cucumber support code: route.js, shared_steps.js,
  • adapt cucumber tests (names.feature, refs.feature, ..) testing the new behavior
  • add api docs for the new ref field in RouteStep to docs/http.md
  • add a changelog entry for both profiles and api change

@MoKob MoKob modified the milestones: 5.5.0, 5.4.0 Sep 6, 2016
@@ -16,7 +16,7 @@ Feature: Bike - Street names in instructions

When I route I should get
| from | to | route |
| a | c | My Way (A6),Your Way (A7),Your Way (A7) |
| a | c | My Way,Your Way,Your Way |
Copy link

Choose a reason for hiding this comment

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

these have to be added as an additional check instead of being removed | route | ref | ... or something like this. References are an essential part of guidance checks.

@emiltin
Copy link
Contributor

emiltin commented Sep 6, 2016

thanks @daniel-j-h for the update

- Guidance
- Handle Access tags for lanes, only considering valid lanes in lane-guidance (think car | car | bike | car)
- API:
- `annotations=true` now returns the data source id for each segment as `datasources`
- Reduced semantic of merge to refer only to merges from a lane onto a motorway-like road
- new `ref` field in the `RouteStep` object
Copy link
Member

Choose a reason for hiding this comment

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

What is is good for? Also maybe a note about "name (ref)" -> name, ref.

| waypoints | route | turns |
| a,o | Schwarzwaldstrasse (L561),Ettlinger Allee,Ettlinger Allee | depart,turn right,arrive |
| waypoints | route | turns | ref |
| a,o | Schwarzwaldstrasse,Ettlinger Allee,Ettlinger Allee | depart,turn right,arrive | L561,L561, |
Copy link
Member

Choose a reason for hiding this comment

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

Before it was a single ref, now it's two refs?

Copy link

Choose a reason for hiding this comment

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

Looks like in post-processing, whenever name gets updated, refs are not getting updated accordingly

Copy link
Contributor Author

@karenzshea karenzshea Sep 7, 2016

Choose a reason for hiding this comment

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

This is actually an existing bug where in post-processing we don't properly update collapsed steps with the correct destination and pronunciation data along with name. PR coming for this fix, which can then include moving ref in preprocessing as well.

#2857

@MoKob MoKob modified the milestones: 5.4.0, 5.5.0 Sep 7, 2016
@MoKob
Copy link

MoKob commented Sep 7, 2016

From discussion in chat. We are trying to include this in 5.4.0. I updated the milestone accordingly

@@ -39,7 +54,7 @@ class ExtractorCallbacks
// used to deduplicate street names and street destinations: actually maps to name ids
using MapKey = std::tuple<std::string, std::string, std::string>;
using MapVal = unsigned;
std::unordered_map<MapKey, MapVal, boost::hash<MapKey>> string_map;
std::unordered_map<MapKey, MapVal, std::hash<MapKey>> string_map;
Copy link
Member

Choose a reason for hiding this comment

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

std::hash is the default, you can remove this. See http://en.cppreference.com/w/cpp/container/unordered_map

@@ -26,7 +26,7 @@ Feature: Foot - Way ref

When I route I should get
| from | to | route |
| a | b | E7,E7 |
| a | b | {highway:primary},{highway:primary} |

Copy link

Choose a reason for hiding this comment

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

align table

@MoKob MoKob merged commit dcc1b5a into master Sep 8, 2016
@MoKob MoKob deleted the refxit branch September 8, 2016 12:53
daniel-j-h added a commit that referenced this pull request Sep 20, 2016
With @karenzshea's name / ref split (ref. #2857) in master we want to
make use of it and reduce `NewName` instructions when ever possible.
This is a first step towards #2744 by using the already existing name
change heuristic from the extractor now in post-processing as well.

Limitations: at the moment we don't have the `SuffixTable` in
post-processing; this would require us serializing and subsequently
deserializing the table, passing it through from the profiles to the
API.
MoKob pushed a commit that referenced this pull request Sep 21, 2016
With @karenzshea's name / ref split (ref. #2857) in master we want to
make use of it and reduce `NewName` instructions when ever possible.
This is a first step towards #2744 by using the already existing name
change heuristic from the extractor now in post-processing as well.

Limitations: at the moment we don't have the `SuffixTable` in
post-processing; this would require us serializing and subsequently
deserializing the table, passing it through from the profiles to the
API.
MoKob pushed a commit that referenced this pull request Sep 21, 2016
With @karenzshea's name / ref split (ref. #2857) in master we want to
make use of it and reduce `NewName` instructions when ever possible.
This is a first step towards #2744 by using the already existing name
change heuristic from the extractor now in post-processing as well.

Limitations: at the moment we don't have the `SuffixTable` in
post-processing; this would require us serializing and subsequently
deserializing the table, passing it through from the profiles to the
API.
MoKob pushed a commit that referenced this pull request Sep 21, 2016
With @karenzshea's name / ref split (ref. #2857) in master we want to
make use of it and reduce `NewName` instructions when ever possible.
This is a first step towards #2744 by using the already existing name
change heuristic from the extractor now in post-processing as well.

Limitations: at the moment we don't have the `SuffixTable` in
post-processing; this would require us serializing and subsequently
deserializing the table, passing it through from the profiles to the
API.
daniel-j-h added a commit that referenced this pull request Sep 22, 2016
daniel-j-h added a commit that referenced this pull request Sep 28, 2016
daniel-j-h added a commit that referenced this pull request Sep 29, 2016
daniel-j-h added a commit that referenced this pull request Sep 29, 2016
daniel-j-h added a commit that referenced this pull request Oct 5, 2016
daniel-j-h added a commit that referenced this pull request Oct 10, 2016
daniel-j-h added a commit that referenced this pull request Oct 12, 2016
MoKob pushed a commit that referenced this pull request Oct 13, 2016
daniel-j-h added a commit that referenced this pull request Oct 13, 2016
daniel-j-h added a commit that referenced this pull request Oct 14, 2016
daniel-j-h added a commit that referenced this pull request Oct 25, 2016
daniel-j-h added a commit that referenced this pull request Nov 7, 2016
MoKob pushed a commit that referenced this pull request Nov 8, 2016
daniel-j-h added a commit that referenced this pull request Nov 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants