No cache option for PyCrest __init__ and get() calls

[Kyria]

As i stated in #44, there were no solutions to prevent pycrest to do some caching, even by stating cache=None

This PR adds this features.

More precisely, for cache definition in EVE class:

• current default behavior is kept: DictCache is the default cache
• if you explicitly set cache=None you will use the DummyCache which does nothing but expose expected caching methods (get(), put(), invalidate()) not to break the existing code with if cache==None everywhere

Concerning the get() calls:

• if you add caching=False or caching=None, it will only not store the result in the cache, but you will still use the existing cache, if available.
• I also added this parameter to ApiConnection and AuthedConnection call, for homogeneity purpose.

The doc is updated with examples.

[coveralls]

Coverage remained the same at 100.0% when pulling 41ddf7b on Kyria:no-cache-option into 9b541ec on pycrest:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 3aa0d26 on Kyria:no-cache-option into 9b541ec on pycrest:master.

[jonobrien]

This looks great!

[hkraal]

@Kyria awesome, I'll take a closer look before merging this weekend.

@wtfrank if you could drop an eye if you can find the time as well it will be much appriciated

[wtfrank]

This isn't really your changes, but I think that whole cache vs cache_dir thing is a mess.

You can pass in via the cache argument an instance of a class that derives from APICache, and it will use that as a cache. Or you can pass in the name of a class which may or may not derive from APICache and it will instantiate an object of this type and use it as a cache. Or you can pass in a directory via cache_dir and it will instantiate a FileCache with that directory. But what if you pass in both a cache and a cache_dir? What should it do? This pull changes the priority of cache_dir and cache unnecessarily, which possibly breaks existing dodgy code. But noone should be passing in both objects so it doesn't really matter.

However I think the entire interface would be much neater if we got rid of the historic cache_dir argument (or maybe deprecated it and printed an annoying warning), because there shouldn't be two different ways of specifying what cache you want.

I'm not sure what people think about the ability to pass in the type of a class rather than a class object? But seeing as you can't specify any arguments when you do this, and the current implementation doesn't even guarantee that you end up with an object that has get/put/invalidate methods on it, I think it needs to be changed.

I think there should be a single argument (cache) which accepts an instance of an object that inherits from APICache. If we retain the ability to pass in the type of an object, then we would need a check that ensures the object we instantiate actually inherits from APICache. But I'm not sure of the benefit of being able to pass in the type.

This probably needs a new issue to deal with though cos its only a little bit about this pull request and more about the interface in general.

[Kyria]

Yep, i totally agree with that. While I did these changes I actually though about it (cache/cache_dir and the cache type to instanciate).

I had to change the order (cache_dir vs cache) in the if statement as I changed the default value of "cache" (as it's now a DictCache() by default, even with cache_dir, it was always a DictCache).

I believe the best things to do are:

• for the FileCache, to remove the cache_dir stuff, let the people create their own FileCache the way they want it
• remove the create fact we create on the fly the cache object if we get just a type
• check if the given class object is an instance of ApiCache, to be sure we have the required methods defined.

I know it may break compatibility to older versions, but it wasn't the best design, as in some case we had non trivial choice being mad arbitrary with the current code.

Quickly, something like that:

if cache and isinstance(cache, APICache):
    # use the given cache, as we are sure it's OK
    self.cache = cache  
elif cache is None:
    # Dummy if no cache
    self.cache = DummyCache()
else:
    # if we are here, it's because the user gave a cache (he wants some cache)
    # but it was not the correct type. 
    raise ValueError('Given cache must implement APICache')

Why an exception ? Because I believe we shouldn't use a "random cache" when the user wanted to use one for his usecase, so we have to tell him he did it wrong, thus I think returning an exception is the best case.

[wtfrank]

that logic looks reasonable to me

[Kyria]

@hkraal if you agree with this logic (and if you want) I can add this to my PR, so you can add it both. If not, i'll make an issue of this, not to lose tracks and as a reminder

[hkraal]

@Kyria Sorry, somehow I missed the mention. Please create an issue to address the current cache implementation in the future, for now this works which is good enough. From my point of view it should be a nice feature update for v2.x or something