search

moshen / node-googlemaps

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

Enable multiple style parameters in query string. #87

Closed felix closed 8 years ago

felix commented 9 years ago

This requires the 'qs' library which does more than the Node 'querystring' enabling us to spread the style array out nicely.

Updated the test as well.

rayshan commented 9 years ago

Thanks @felix, I've tested it and works as expected. Failed tests are only in node 0.8, probably ok. Can't really comment on the code style as I'm not familiar with the code base.

felix commented 9 years ago

Let me know if code style has to change. Also not sure about travis failures, let me know what I need to get them to work.

moshen commented 9 years ago

Probably should just remove node 0.8 from testing, quite old now. @fabriziomoscon ?

ShayDavidson commented 8 years ago

Is the fork where this is fixed published in NPM? I'd really like to use this without having to wait for this to be merged.

ShayDavidson commented 8 years ago

What I actually meant to ask, (since I can reference that fork in my package.json), is when this is going to be merged, since without this static maps style are pretty broken

ShayDavidson commented 8 years ago

What I actually meant to ask, (since I can reference that fork in my package.json), is when this is going to be merged, since without this static maps style are pretty broken

hueitan commented 8 years ago

This looks good :+1:

fabriziomoscon commented 8 years ago

I still haven't got a chance to test this work personally.

Although it looks like this PR changes the expected results for this test: https://github.com/felix/node-googlemaps/blob/86-single-style/test/unit/utils/parseStylesTest.js#L48

I understand qs helps with nesting of parameters, but because of the change in the test it is not possible to ensure this change won't break other users' code.

felix commented 8 years ago

Been a while since this pull request started. It seems the change to the parseStylesTest is necessary to test the whole point of this PR. If parseStyles is actually part of the public API then yes, it may break others' code, but it is broken anyway.

fabriziomoscon commented 8 years ago

Ok, I tested your changes and they look good. Please amend these blocks to make them consistent with the new return type of the function https://github.com/felix/node-googlemaps/blob/86-single-style/test/unit/utils/parseStylesTest.js#L30 https://github.com/moshen/node-googlemaps/blob/master/lib%2Futils%2FparseStyles.js#L2,L15

and then it is good to be merged

fabriziomoscon commented 8 years ago

@felix @rayshan this is now published on npm under version 1.2.0

rayshan commented 8 years ago

:tada: thanks @felix @fabriziomoscon & team!