eve-val / evelink

Python bindings for the EVE API.
Other
91 stars 30 forks source link

Allows to set a default cache class. #191

Closed bastianh closed 9 years ago

ayust commented 9 years ago

Hm. If we're going to support this in this fashion, we probably shouldn't be using a _variable - the underscore prefix is usually designed to indicate something internal that isn't considered a supported public interface.

The alternative option for doing this sort of thing is to subclass and override the logic. We could make that easier by breaking out default cache selection into a method of the API object that would be easier to override. However, this wouldn't affect the auto_api behavior since it explicitly instantiates API() directly.

Do you think people who are using this are likely to be taking advantage of auto_api or more explicitly constructing their API objects?

bastianh commented 9 years ago

I'm not sure what the best option would be. I was previously using a setup function to set the default cache object, but switched to the underscore thing since it is already used for the user agent.

I think it's important to do stuff like that in a consistent way, learned that the hard way in my project ,)

ayust commented 9 years ago

So in the user agent case, there are two components - there's the "internal" component which is _user_agent which in general shouldn't be touched by other users (it is set by __init__.py to contain the evelink version info, and the variable only really exists to give __init__.py a place to set that info), and then there's the user_agent parameter to API() which allows EVELink users to append additional info to that string.

That's why _user_agent has an underscore prefix - because it's actually not really meant for users of the library so much as the library itself.

bastianh commented 9 years ago

ok .. i think then the comment at the variable is misleading:

# Can be set to specify a custom user agent HTTP header on requests

sounded to me like it is meant to set directly since we are asked now to include contact information in the user agent when we use the eve api.

I'm creating evelink api objects in different files of my application whenever I need them and getting the cache object and setting the user agent at all places is not really nice.

I think setting the cache object once from outside makes it also much easier to write tests, since you don't have to alter the function calls that create the api objects.

ayust commented 9 years ago

I agree that the comment there is sub-par - I'll update it.

I think you've sold me on the usefulness of having a global setting - among other things, it seems unlikely that someone would use multiple different cache types simultaneously. Let's do this with a non-underscore variable.