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 annotations paramater #3684

Closed
chaupow opened this issue Feb 10, 2017 · 3 comments · Fixed by #3692
Closed

Refactor annotations paramater #3684

chaupow opened this issue Feb 10, 2017 · 3 comments · Fixed by #3692
Assignees
Milestone

Comments

@chaupow
Copy link
Member

chaupow commented Feb 10, 2017

Crossposting from Project-OSRM/node-osrm#297:

tl;dr: node-osrm doesn't pass all tests anymore and its probably due to a bug in #3628

screenshot 2017-02-10 11 35 09

https://travis-ci.org/Project-OSRM/node-osrm/builds

tests are failing due to response not having annotations even though the request is set to annotations=true

this is a blocker for 5.6

/cc @danpat @TheMarex

@chaupow chaupow added this to the 5.6.0 milestone Feb 10, 2017
@chaupow chaupow self-assigned this Feb 10, 2017
@chaupow chaupow changed the title Missing annotations in response when annotations=true Refactor annotations paramater Feb 10, 2017
@chaupow
Copy link
Member Author

chaupow commented Feb 10, 2017

In oder to implement #3563
and to not add a breaking API change, we currently have two parameter member variables:

bool annotations
AnnotationsType annotations_type

this makes node-osrm have to handle two parameters. it should be made such that there's only one parameter:

AnnotationsType annotations

while still making sure annotations=true|false doesnt fail.

@daniel-j-h
Copy link
Member

@chaupow this is already ticketed for v6 in #3644.

Until then, node-osrm handles Javascript. Therefore for node-osrm you can handle

annotations=true

keeping the old API and at the same time support

annotations='speed'

and support the new API in Javascript.

@chaupow
Copy link
Member Author

chaupow commented Feb 10, 2017

@chaupow this is already ticketed for v6 in #3644.

awesome. missed this. so this can be closed as the bug is fixable in node-osrmonly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants