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

4 new options for the directions layer (Issue: 151) #153

Merged

Conversation

Projects
None yet
2 participants
@gatapia
Copy link
Contributor

commented Jul 2, 2017

Added avoid_ferries, avoid_highways, avoid_tolls and optimize_waypoints to the python api.

@pbugnion
Copy link
Owner

left a comment

I think this is really great! Thanks for submitting a PR.

I've made a few (style) suggestions inline. Besides those, there's a few flake8 errors that are causing the build system to complain.

You're welcome to tackle those, or you can just leave them and I'll deal with them later.

};

const directionsService = new google.maps.DirectionsService();
const directionsService = new google.maps.DirecdctionsService();

This comment has been minimized.

Copy link
@pbugnion

pbugnion Jul 2, 2017

Owner

Typo here ^ (I don't think this line should have changed)



def directions_layer(start, end, waypoints=None):
def directions_layer(start, end, waypoints=None, **kwargs):

This comment has been minimized.

Copy link
@pbugnion

pbugnion Jul 2, 2017

Owner

I'm in favour of explicitly specifying the kwargs (ie explicitly writing avoid_tolls=False, avoid_highways=False, ....

It's a bit easier to understand for someone reading the code, and since Jupyter and many IDEs offer completion based on the signature, having the kwargs spelled out makes them much more discoverable.

@@ -133,6 +151,23 @@ def directions_layer(start, end, waypoints=None):
Google maps imposes a limitation on the total number of waypoints.
This limit is currently 23.
:type waypoints: List of 2-element tuples, optional
:param avoid_ferries If true, instructs the Distance Matrix service to avoid

This comment has been minimized.

Copy link
@pbugnion

pbugnion Jul 2, 2017

Owner

I think adding which GoogleMaps service is affected by parameters will confuse most users -- how about just "avoid ferries where possible"?

Also, you're missing a colon after the parameter name:

:param avoid_ferries: avoid ferries where possible (default: False)

(the same is true for the other parameters here)

@@ -48,6 +48,15 @@ class Directions(widgets.Widget):
90 degrees north). Longitudes are expressed as a float
between -180 (corresponding to 180 degrees west) and 180
(corresponding to 180 degrees east).
:param avoid_ferries If true, instructs the Distance Matrix service to avoid
ferries where possible.

This comment has been minimized.

Copy link
@pbugnion

pbugnion Jul 2, 2017

Owner

I think adding which GoogleMaps service is affected by parameters will confuse most users -- how about just "avoid ferries where possible"?

Also, you're missing a colon after the parameter name:

:param avoid_ferries: avoid ferries where possible (default: False)

(the same is true for the other parameters here)

Guido Tapia added some commits Jul 4, 2017

@pbugnion
Copy link
Owner

left a comment

Nice! I've made one tiny comment inline, but modulo that, it looks great! Thanks again for submitting this.

def _directions_options(start, end, waypoints):
def _directions_options(start, end, waypoints, avoid_ferries=False,
avoid_highways=False, avoid_tolls=False,
optimize_waypoints=False):

This comment has been minimized.

Copy link
@pbugnion

pbugnion Jul 4, 2017

Owner

Since this only gets called internally and we've explicitly bubbled the defaults up to the directions_layer function, we don't need the default values in the type signature here: _directions_options always gets called with all the arguments explicitly.

I think it therefore makes sense to remove the defaults?

def _directions_options(start, end, waypoints, avoid_ferries, avoid_highways, avoid_tolls, optimize_waypoints)

This comment has been minimized.

Copy link
@gatapia

gatapia Jul 4, 2017

Author Contributor

This causes unit tests to fail when run in travis

This comment has been minimized.

Copy link
@pbugnion

pbugnion Jul 5, 2017

Owner

I've made the tests pass -- in the future, that test should probably be changed to test the widget directly, rather than a private method.

@pbugnion

This comment has been minimized.

Copy link
Owner

commented Jul 5, 2017

Are you happy with this? If so, I'll merge it.

@gatapia

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2017

Yes ofcourse, great widget by the way, it has really helped in my current project (route optimisation project)

@pbugnion

This comment has been minimized.

Copy link
Owner

commented Jul 5, 2017

Awesome, thanks for the contribution.

great widget by the way, it has really helped in my current project (route optimisation project)

Thanks! Good luck.

@pbugnion pbugnion merged commit 64f1a47 into pbugnion:master Jul 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gatapia gatapia deleted the gatapia:additional_params_for_directions_layer branch Jul 5, 2017

@pbugnion pbugnion referenced this pull request Jul 8, 2017

Merged

Release version 0.5.3 #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.