Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP connection pooling #75

Open
masellfoodpanda opened this issue Sep 6, 2016 · 5 comments
Open

HTTP connection pooling #75

masellfoodpanda opened this issue Sep 6, 2016 · 5 comments

Comments

@masellfoodpanda
Copy link

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
Copy link

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

@jonathan-golorry
Copy link

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
Copy link
Owner

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
Copy link

jonathan-golorry commented Jan 9, 2017

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
Copy link
Owner

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants