search

slimkrazy / python-google-places

Simple wrapper around the new 'experimental' Google Places API
MIT License
449 stars 168 forks source link

HTTP connection pooling #75

Open masellfoodpanda opened 8 years ago

masellfoodpanda commented 8 years ago

Hey.

It would be great if you could abstract away the urllib dependency to allow to choose different transports. That would allow for example to use requests HTTPConnectionPool which would improve performance a lot i think.

jonathan-golorry commented 7 years ago

I'll second this simply because I find urllib extremely unreliable.

jonathan-golorry commented 7 years ago

I did some tinkering and found that get_details is failing pretty frequently (connection reset by peer roughly 1 in 4 times). I rewrote the key logic to use requests and haven't had an error yet.

I don't think I have the expertise to abstract away which networking package is used, but if other people are running into this issue we could switch over to requests entirely.

slimkrazy commented 7 years ago

Hey @jonathan-golorry,

The original intent was for this library to not require any additional dependencies (its function is pretty simple, after all).

Potentially we could manage to keep this and support the use of requests by simply extending the init API to accept the requests module, then the rest of this library can use duck-typing to figure out if the module can deal with making http requests. If it does, let that module deal with it, otherwise fall back to urllib.

Thoughts?

jonathan-golorry commented 7 years ago

Makes sense. I can easily overwrite the _fetch_remote methods during initialization based on a few hasattr checks on the passed module. My only worry would be that anyone importing the _fetch_remote functions would need to do so after initializing the GooglePlaces object. The alternative would be to move most of the global functions into the GooglePlaces class so they are only available after initialization.

Update: I got a bit confused by the code and was wondering why geocode_location was even a global method in the first place. Looks like it is using the IP-based 2500 query/day limit instead of providing the API key given during initialization. Is this intentional?

slimkrazy commented 7 years ago

Honestly, I cannot recall what the original intent was for allowing geocoding requests to be made without the API key.

I wouldn't worry about _fetch_remote being imported by somebody. PEP8: "Public attributes should have no leading underscores".

This work can include changes to address issue #64 as well, right?