search

barnumbirr / coinmarketcap

A python wrapper around the https://coinmarketcap.com API.
Apache License 2.0
437 stars 110 forks source link

Allow saving cache data to a temp directory #31

Closed jathpala closed 6 years ago

jathpala commented 6 years ago

Adds a 'tempdir_cache' boolean option to the Market class which allows the cached market data to be saved to the system temp directory rather than the current working directory. This avoids polluting the working directory with a short-term file that the user likely doesn't need to be able to directly access themselves.

By default the tempdir is not used (for backwards compatibility) and has to be explicitly requested by passing 'tempdir_cache = True' when instantiating a Market object.

An alternative approach would be to just allow the user to specify the filename and location they want to store the file in but given that 99% of the time I imagine people wouldn't really care, it is probably simpler to have it as a boolean option to put it in the temp directory (or make it default if backwards compatibility is not a concern).

jathpala commented 6 years ago

Is this what you mean?

jfunction commented 6 years ago

Woohoo - I was actually about to write this myself. Keen for it to be pushed through - any issues @mrsmn? It's just a little strange sometimes having a sqlite file in the repo and sometimes not. Throwing it in a tmp directory solves that.

I'm not sure __TEMPDIR_CACHE=False should be the default? Surely you always want this to go to a tmp directory? I'm not convinced that breaking backwards compatibility is such a bad thing here. In the worst case you get a (small) stale cache file. Obviously not my place to say, just giving my 10c worth :)

Thanks for the library - I've enjoyed using it. And thanks @jathpala for the PR.

barnumbirr commented 6 years ago

Hey there,

I actually kind of agree with @jfunction on this one, I think it might be a good thing for __TEMPDIR_CACHEto be set to True by default as the stale cache file is not really an issue. What do you think @jathpala? (thanks for the PR by the way, much appreciated 👍)

Cheers.

jathpala commented 6 years ago

Hi,

Yeah I'd be very happy for it to be the default. It does certainly make more sense and I doubt anyone using the library would miss having the cache file in their working directory. I'll push the modified changes through in a minute.

And very happy to help. Thanks for the library in the first place.

barnumbirr commented 6 years ago

Merged! Again, PR is much appreciated @jathpala 👍