moshen / node-googlemaps

A simple way to query the Google Maps API from Node.js
MIT License
560 stars 148 forks source link

Added Street View API #10

Closed spatical closed 12 years ago

spatical commented 12 years ago

I've added an additional method for the Street View static image API. It follows the same parameter format as your existing functions and I also added some test cases. It appears the MD5 of the returned images doesn't stay consistent from google over time, so test cases for actual image data might need to be re-factored later.

Exports an additional method setBusinessSpecificParameters that can be used to pass a Google Maps for Business account parameters. When set the makeRequest calls a new function signGoogleRestApiRequest that adds the client and signature parameters to the request url before requesting it from Google. Valid Google Maps for Business parameters are required in order for the request to actually return data.

moshen commented 12 years ago

Looks great! I'll integrate in the next day or two. FYI, you probably only needed one pull request unless you had separate feature branches.

spatical commented 12 years ago

You are right. You only need to consider one of these pull requests.

moshen commented 12 years ago

I took a few minutes to clone your repo and take a look, and there are a few strange things that happen. The setBusinessSpecificParameters function is fired for every request when running the tests, though I'm not sure why. Can you confirm its functionality?

Thanks

spatical commented 12 years ago

setBusinessSpecificParameters is used for setting up your google maps API client ID and private key. This will be useful to any users/applications that are exceeding the limits set by Google for their API. This is used with the signGoogleRestApiRequest function that occurs on every request if the client ID and private key have been set.

I created a couple of test cases involving the client Id and key, but it shouldn't be calling it for every test.

moshen commented 12 years ago

I understand why you would want to test it. When I run the tests on a clone of your repo it fails almost every test because all the urls have the clientId & signature attached to them (when it looks like they shouldn't).

The problem could be my environment ( but, my fork passes the existing tests ). Could you double check your fork?

moshen commented 12 years ago

I checked your fork again for any other changes. For me, the tests start failing when it gets to reverseGeocode... because the JSON.parse bails... because it receives an error message about the invalid signature from google.

Are you testing with a valid key, or as is in the ifit repo?

regality commented 12 years ago

Google only allows the url to be 2048 characters, or that request will fail. This will break up requests sent to elevationFromPath() if they exceed the max url length.

That commit should fix the issue with tests failing. The test for PNGs is not working, but I'm assuming that the image google returns has changed.

It also cleans up a few things, removes some repetitous code, and makes the whitespace consistent throughout the code.

moshen commented 12 years ago

Looks great, tests pass, but you remove args.sensor in a number of places. Why? It's never added back, and as far as I can tell, makeRequest will set it to false every time.

regality commented 12 years ago

Sorry about that, not sure what I was thinking. That last commit puts sensor back into args.

moshen commented 12 years ago

No worries. I was wondering if I missed something. Will test again and probably merge this time.

grobot commented 12 years ago

Added in ability to encode polylines as per Google's benevolent spec seen here: https://developers.google.com/maps/documentation/utilities/polylinealgorithm

Also, changed how businessSpecificParameters is handled. We are using a config setting instead. This is documented in the README.md.

Also, would you publish this to npm?

moshen commented 12 years ago

Will do, I merged it into my local repo, will tag and push today/tomorrow.

moshen commented 12 years ago

Merged and pushed a new release. Thanks ifit guys! :+1: