search

moshen / node-googlemaps

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

handle a console-key parameter #15 #38

Closed chevett closed 10 years ago

chevett commented 10 years ago

now i can be like

var gm = require('googlemaps');
gm.config('console-key', 'xj555');

and my urls will be like https://google/api/...&key=xj555

Additionally, it is necessary to use https when the key is included, so this forces https when there is a key.

grobot commented 10 years ago

Code looks good.

What do you think about adding a test or two?

chevett commented 10 years ago

I thought you guys may say that :) I'll give it a shot tonight.

fredx21 commented 10 years ago

Not sure a test can be written cause you'd have to actually log into the developers console and provide some legit credentials right?

I just tested his patch and it worked perfectly fine for me. Might be time to merge!

chevett commented 10 years ago

fwiw, i've been using my fork for months with no problems. i think the only valid option is to just ensure the url includes the key when set, but I'm not familiar enough with this code to get that test written without some digging.

fabriziomoscon commented 10 years ago

@chevett I cannot see a good reason not to merge this code. Please rebase in top of master (since the code has changed since your addition) so I can merge your code automatically.

chevett commented 10 years ago

@fabriziomoscon I rebased and it now appears to fail with node 0.8.28. I switched to node 0.8.28 locally and somehow I can't reproduce. Do you have the ability to repeat the build on travis before I start digging further?

chevett commented 10 years ago

oh nevermind, i figured out how to trigger it again...

chevett commented 10 years ago

hrmm, gremlins i guess. it's ready to merge when you are.

fabriziomoscon commented 10 years ago

@chevett the test in place in this library are not unit, but integration. As such they do require the network to pass and also they can break for ANY network failure. This is not something we should let TravisCI rely upon, but it is in plan to write unit test and let the integration as manual facility.