Allow speed_unit to be passed to request #7

netuoso · 2017-05-10T21:00:45Z

Explanation:

This change leaves KPH as the default while adding the ability to send 'MPH' to the roads.rb

Example request:

google_client = GoogleMapsService::Client.new
google_client.speed_limits('ChIJaw1TDuC22YgRf6hEN94cT58')
=> [{:placeId=>"ChIJaw1TDuC22YgRf6hEN94cT58", :speedLimit=>88.51392, :units=>"KPH"}]

google_client = GoogleMapsService::Client.new
google_client.speed_limits('ChIJaw1TDuC22YgRf6hEN94cT58', units: 'MPH')
=> [{:placeId=>"ChIJaw1TDuC22YgRf6hEN94cT58", :speedLimit=>55, :units=>"MPH"}]

coveralls · 2017-05-10T21:05:44Z

Coverage remained the same at 100.0% when pulling 4e86596 on netuoso:master into 46746fd on edwardsamuel:master.

coveralls · 2017-05-10T21:05:44Z

Coverage remained the same at 100.0% when pulling 4e86596 on netuoso:master into 46746fd on edwardsamuel:master.

Since KPH is google default unit when no unit param is sent, just do not send it at all unless speed_unit is set to MPH

coveralls · 2017-05-11T16:17:00Z

Coverage remained the same at 100.0% when pulling 3a75f10 on netuoso:master into 46746fd on edwardsamuel:master.

coveralls · 2017-05-11T16:17:00Z

Coverage remained the same at 100.0% when pulling 3a75f10 on netuoso:master into 46746fd on edwardsamuel:master.

coveralls · 2017-05-11T16:17:00Z

Coverage remained the same at 100.0% when pulling 3a75f10 on netuoso:master into 46746fd on edwardsamuel:master.

edwardsamuel · 2017-05-12T14:57:18Z

Hi @netuoso
Thanks for your PR. I just saw that Speed Limit have units.
Before I merge, I have some reviews on yours (I'll write in separate comments).

edwardsamuel · 2017-05-12T15:00:32Z

lib/google_maps_service/apis/roads.rb

@@ -63,8 +63,9 @@ def snap_to_roads(path, interpolate: false)
    #         by the snap_to_roads function. You can pass up to 100 Place IDs.
    #
    # @return [Array] Array of speed limits.
-    def speed_limits(place_ids)
+    def speed_limits(place_ids, speed_unit='KPH')


I prefer to use units, instead of speed_unit. Here I want to keep the params similar to the Google Maps request params name.

And also, please use :, instead of =, to keep consistent with other methods.

There is snapped_speed_limits method that can accept units also, do you mind to add this for that method?

I think your example in the PR description can be put in the method example here, so it will be in documentation also :)

Sure thing. I will update this later today so you can check it over! Thanks for the gem btw. Very useful.

edwardsamuel · 2017-05-12T15:02:50Z

lib/google_maps_service/apis/roads.rb

      params = GoogleMapsService::Convert.as_list(place_ids).map { |place_id| ['placeId', place_id] }
+      params << ['units', speed_unit] if speed_unit == 'MPH'


Same as before, to keep it similar to other methods
Could you please change to: params[:units] = ...?

In this situation, params is an array. [['key1','value1'],['key2','value2']]. I went ahead and made the conversion from the multi-dimensional array to a hash. Would you rather it converted to a hash for readability or to use the << or push methods to add the the array?

snapped_speed_limits params are in hash form so this isn't a concern there.

Fyi, when i converted the params to a hash and added the units as a symbol, it created problems. Therefore I went with params['units'] instead inside speed_limits method.

coveralls · 2017-05-12T16:05:08Z

Coverage decreased (-0.3%) to 99.73% when pulling ce77462 on netuoso:master into 46746fd on edwardsamuel:master.

coveralls · 2017-05-12T16:05:08Z

Coverage decreased (-0.3%) to 99.73% when pulling ce77462 on netuoso:master into 46746fd on edwardsamuel:master.

coveralls · 2017-05-12T16:05:08Z

Coverage decreased (-0.3%) to 99.73% when pulling ce77462 on netuoso:master into 46746fd on edwardsamuel:master.

coveralls · 2017-05-12T16:23:05Z

Coverage decreased (-0.3%) to 99.73% when pulling 02973b7 on netuoso:master into 46746fd on edwardsamuel:master.

coveralls · 2017-05-12T16:23:05Z

Coverage decreased (-0.3%) to 99.73% when pulling 02973b7 on netuoso:master into 46746fd on edwardsamuel:master.

coveralls · 2017-05-12T16:23:05Z

Coverage decreased (-0.3%) to 99.73% when pulling 02973b7 on netuoso:master into 46746fd on edwardsamuel:master.

netuoso · 2017-05-12T16:25:22Z

@edwardsamuel ok waiting for you to review. the coverage decrease is due to changing the params sent to the generate_auth_url method in client.rb since it isn't receiving an array from the speed_limits method any longer.

if we wrapped the array to hash conversion inside a conditional that only executes if units is sent as a param to the speed_limits method it would retain 100% coverage.

coveralls · 2017-05-12T16:34:50Z

Coverage decreased (-0.3%) to 99.73% when pulling 9c20a42 on netuoso:master into 46746fd on edwardsamuel:master.

coveralls · 2017-05-12T16:34:50Z

Coverage decreased (-0.3%) to 99.73% when pulling 9c20a42 on netuoso:master into 46746fd on edwardsamuel:master.

edwardsamuel · 2017-05-13T04:24:48Z

Hi @netuoso,
Sorry, my bad. I realize that we can't use Hash params in the speed_limits method, but it's fine on snapped_speed_limits.
You are right, use array instead:

params << ['units', units] if units.match(/^mph$/i)

and no need to do:

params = Hash[params]

Thanks!

netuoso · 2017-05-15T15:23:01Z

@edwardsamuel No problem! That should do it.

coveralls · 2017-05-15T15:29:03Z

Coverage remained the same at 100.0% when pulling 7493b02 on netuoso:master into 46746fd on edwardsamuel:master.

coveralls · 2017-05-15T15:29:03Z

Coverage remained the same at 100.0% when pulling 7493b02 on netuoso:master into 46746fd on edwardsamuel:master.

coveralls · 2017-05-15T15:29:05Z

Coverage remained the same at 100.0% when pulling 7493b02 on netuoso:master into 46746fd on edwardsamuel:master.

edwardsamuel

LGTM! Thanks @netuoso

edwardsamuel · 2017-05-15T15:43:25Z

Will merge and publish it soon

rnestler · 2022-09-20T13:11:48Z

Will merge and publish it soon

What happened to this?

Allow speed_unit to be passed to request

4e86596

netuoso mentioned this pull request May 10, 2017

Unable to select preferred unit for speed_limits request #8

Open

Make update to keep existing tests passing

3a75f10

Since KPH is google default unit when no unit param is sent, just do not send it at all unless speed_unit is set to MPH

edwardsamuel requested changes May 12, 2017

View reviewed changes

netuoso added 2 commits May 12, 2017 10:59

Make changes as requested by edwardsamuel

684c512

Merge branch 'master' of github.com:netuoso/google-maps-services-ruby

ce77462

Change syntax to map array to Hash to support Ruby2.0.0

02973b7

Cleaner syntax for Array to Hash

9c20a42

Update roads.rb

7493b02

edwardsamuel approved these changes May 15, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow speed_unit to be passed to request #7

Allow speed_unit to be passed to request #7

netuoso commented May 10, 2017 •

edited

Loading

coveralls commented May 10, 2017

coveralls commented May 10, 2017 •

edited

Loading

coveralls commented May 11, 2017

coveralls commented May 11, 2017

coveralls commented May 11, 2017

edwardsamuel commented May 12, 2017

edwardsamuel May 12, 2017

edwardsamuel May 12, 2017

edwardsamuel May 12, 2017

netuoso May 12, 2017

edwardsamuel May 12, 2017

netuoso May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

netuoso commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

edwardsamuel commented May 13, 2017

netuoso commented May 15, 2017

coveralls commented May 15, 2017

coveralls commented May 15, 2017

coveralls commented May 15, 2017

edwardsamuel left a comment

edwardsamuel commented May 15, 2017

rnestler commented Sep 20, 2022

		params = GoogleMapsService::Convert.as_list(place_ids).map { \|place_id\| ['placeId', place_id] }
		params << ['units', speed_unit] if speed_unit == 'MPH'

Allow speed_unit to be passed to request #7

Are you sure you want to change the base?

Allow speed_unit to be passed to request #7

Conversation

netuoso commented May 10, 2017 • edited Loading

Explanation:

Example request:

coveralls commented May 10, 2017

coveralls commented May 10, 2017 • edited Loading

coveralls commented May 11, 2017

coveralls commented May 11, 2017

coveralls commented May 11, 2017

edwardsamuel commented May 12, 2017

edwardsamuel May 12, 2017

Choose a reason for hiding this comment

edwardsamuel May 12, 2017

Choose a reason for hiding this comment

edwardsamuel May 12, 2017

Choose a reason for hiding this comment

netuoso May 12, 2017

Choose a reason for hiding this comment

edwardsamuel May 12, 2017

Choose a reason for hiding this comment

netuoso May 12, 2017

Choose a reason for hiding this comment

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

netuoso commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

edwardsamuel commented May 13, 2017

netuoso commented May 15, 2017

coveralls commented May 15, 2017

coveralls commented May 15, 2017

coveralls commented May 15, 2017

edwardsamuel left a comment

Choose a reason for hiding this comment

edwardsamuel commented May 15, 2017

rnestler commented Sep 20, 2022

netuoso commented May 10, 2017 •

edited

Loading

coveralls commented May 10, 2017 •

edited

Loading