Skip to content
This repository has been archived by the owner on Jun 8, 2024. It is now read-only.

Wrap: Allow annotations parameter to accept specific fields #298

Closed
3 tasks
daniel-j-h opened this issue Feb 10, 2017 · 7 comments
Closed
3 tasks

Wrap: Allow annotations parameter to accept specific fields #298

daniel-j-h opened this issue Feb 10, 2017 · 7 comments
Assignees
Labels
Milestone

Comments

@daniel-j-h
Copy link
Member

The annotations parameter changed form a bool to specific annotation types.
We need to wrap this in node-osrm, too.

Upstream:

Tasks

  • node_osrm.cpp adapt comments
  • node_osrm_support.hpp parse from Javascript-land
  • adapt docs/api.md

Here's the entry point where we throw an exception currently.

cc @karenzshea

@daniel-j-h daniel-j-h added this to the 5.6 milestone Feb 10, 2017
@karenzshea karenzshea self-assigned this Feb 10, 2017
@karenzshea
Copy link
Contributor

I'll hit this after the speed param has also been added as an annotations option.

@daniel-j-h
Copy link
Member Author

In order to not break the ABI here's what we have to do:

  • support the old boolean parameter
  • allow new string parameters

This is Javascript-land, we can do this by means of checking v8::Value::IsBoolean (old behavior) and v8::Value::IsString() (new behavior).

@chaupow
Copy link
Member

chaupow commented Feb 10, 2017

How should the API look like?
currently I was thinking of supporting following params:

var options = {
    annotations: 'true'
};

var options = {
    annotations: true
};

var options = {
    annotations: ['distance', 'weight', 'node']
};

does this make sense?

@karenzshea @TheMarex @daniel-j-h @freenerd

@TheMarex
Copy link
Member

@chaupow I would suggest not allowing string values but only boolean and array of strings to keep the API surface small.

@chaupow
Copy link
Member

chaupow commented Feb 13, 2017

So shall we accept bool or an array of string?

Then, I will accept

var options = {
    annotations: true
};
var options = {
    annotations: ['distance', 'weight', 'node']
};
var options = {
    annotations: ['distance']
};

I will not accept

var options = {
    annotations: 'distance'
};
var options = {
    annotations: [true]
};

?

@karenzshea @TheMarex

@TheMarex
Copy link
Member

@chaupow exactly.

@chaupow
Copy link
Member

chaupow commented Feb 15, 2017

merged #299

@chaupow chaupow closed this as completed Feb 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants