graphhopper / directions-api-js-client

JavaScript client for the GraphHopper Directions API
https://graphhopper.com/api/1/examples
108 stars 85 forks source link

Manage avoid parameter #36

Closed AlainFolletete closed 6 years ago

AlainFolletete commented 6 years ago

Hi,

Like this topic : https://discuss.graphhopper.com/t/new-routing-api-feature-path-details-and-support-for-avoiding-motorway-ferry-toll/2539

We can add "avoid" parameter to request, this allows to avoid motorway, toll, ferry, ...

Regards,

boldtrn commented 6 years ago

Thank you very much for this PR @AlainFolletete.

Would you mind to add a test case for this feature in the GraphHopperRoutingSpec.js? The avoid feature should be used similar to for example the point_hint parameter, because it is allowed to specify avoid=toll&avoid=motorway. Therefore in js we should specify the avoid parameter as an array. (this is wrong)

AlainFolletete commented 6 years ago

Hi @boldtrn ,

Thank you for your reply. I just push two new commits.

First one is for using "avoid" parameter as array. I create only one "avoid" parameter in url, like is in documentation (https://graphhopper.com/api/1/docs/routing/#flexible).

semicolon separated parameter

The second commit, add a test case in order to test "avoid" parameter is working. I cannot make test working with my API key... I don't know is it working correctly 😢 .

Regards

boldtrn commented 6 years ago

You are right sorry, I just double checked. The avoid parameter is semicolon separated and not exactly like point_hint. I should have looked this up. In this case your initial implementation would have worked as well.

I think I would still prefer the array solution. WDYT @karussell should the avoid parameter be a simple string or an array for the js client?

The second commit, add a test case in order to test "avoid" parameter is working. I cannot make test working with my API key... I don't know is it working correctly

I just created another PR to fix the API key issue #37.

karussell commented 6 years ago

Yes, this inconsistency is a bit ugly. Related to https://github.com/graphhopper/graphhopper/issues/1241

But having the avoid parameter as array is also a bit ugly as we need to skip it explicitly when reading the URL (somewhere)

boldtrn commented 6 years ago

But having the avoid parameter as array is also a bit ugly as we need to skip it explicitly when reading the URL (somewhere)

Yes but this is an issue of the GraphHopper maps demo and not really an issue of the client. Having it as an array also allows to put the parameter as a single string like avoid = ["motorway;toll"] or avoid = ["motorway", "toll"], so we would not need to use it as an array for the GHMaps demo.

karussell commented 6 years ago

Yes but this is an issue of the GraphHopper maps demo and not really an issue of the client

yes, right

Having it as an array also allows to put the parameter as a single string

Sounds interesting, what do you mean exactly?

(btw: we allow avoid=motorway;toll or if this is wrong we should allow this like with the details...)

boldtrn commented 6 years ago

Sounds interesting, what do you mean exactly?

In order to avoid motorway and toll, you can either call:

ghRouting.doRequest({ch: {disable: true}, avoid: [ 'motorway', 'toll' ]})

or:

ghRouting.doRequest({ch: {disable: true}, avoid: [ 'motorway;toll' ]})

(btw: we allow avoid=motorway;toll or if this is wrong we should allow this like with the details...)

Wouldn't say it's wrong, it's just different to most of the other parameters, related to the issue you posted above.

karussell commented 6 years ago

avoid: [ 'motorway;toll' ]

This looks wrong. Let's use a real array of strings.

boldtrn commented 6 years ago

This looks wrong. Let's use a real array of strings.

Ok, that's no problem. Will merge this PR now and release new version on npm.

boldtrn commented 6 years ago

Thanks again @AlainFolletete for providing this PR.