PokeAPI / pokebase

Python 3 wrapper for Pokéapi v2
BSD 3-Clause "New" or "Revised" License
297 stars 54 forks source link

Add (caching) support for LocationAreaEncounters #10

Open jrubinator opened 6 years ago

jrubinator commented 6 years ago

The pokeapi has an attribute that doesn't follow their typical NamedAPIResource pattern: LocationAreaEncounters. Per the docs, when looking up a pokemon's location_area_encounters, it returns a string (URL to list LocationAreaEncounter).

It would be really awesome if pokebase's API implemented lookup and caching for this nonstandard resource. I glanced over the code in hopes of submitting a PR, but as a relative python n00b, I'm having some trouble gauging how best to do this. I'd rather not re-implement caching for this API endpoint outside pokebase.

jrubinator commented 6 years ago

FYI I requested background from the friendly folks on slack

GregHilmes commented 6 years ago

So here's the funny part ... the way location_area_encounters are shown in the PokeAPI docs, they should be automatically handled by creating NamedAPIResource objects (now APIResource objects). But they are implemented differently, so they are not handled by make_obj, hence why they need special handling now that I hadn't accounted for.

I've filed an issue on PokeAPI, but in the meantime I'll add special handling for them, to be included in the next release. (And then hopefully I can get a clear answer on what PokeAPI's planned way to handle them is, and clean up the wrapper code and the PokeAPI docs)

Good catch and thanks for the help! I'll close this issue once I write the fix and push the next release to PyPI.

jrubinator commented 6 years ago

@GregHilmes - so I was looking at the current state of the changes, and noticed that there's a significant performance drop off from the changes:

Namely, once the cache is set, retrieving a pokemon takes an order of magnitude longer (jumping from taking ~.25s to ~3s).

 ~/programming/pokebase (pre-refactor)$ time python3 speed-test-old.py 

real    0m41.041s
user    0m0.796s
sys 0m0.065s
 ~/programming/pokebase (pre-refactor)$ time python3 speed-test-old.py 

real    0m0.241s
user    0m0.217s
sys 0m0.021s
 ~/programming/pokebase (pre-refactor)$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
 ~/programming/pokebase (master)$ time python3 speed-test.py 

real    0m31.404s
user    0m1.572s
sys 0m0.546s
 ~/programming/pokebase (master)$ time python3 speed-test.py 

real    0m3.095s
user    0m0.940s
sys 0m0.482s
 ~/programming/pokebase (master)$ cat speed-test.py 
import pokebase
from pokebase.cache import set_cache

set_cache('speed-test-cache')

pokebase.pokemon('jigglypuff')
 ~/programming/pokebase (master)$ cat speed-test-old.py 
import pokebase
from pokebase.api import set_cache

set_cache('speed-test-cache-old')

pokebase.pokemon('jigglypuff')

Let me know if you want a separate issue for that or anything.

GregHilmes commented 6 years ago

Good catch!

This should definitely be a separate issue - it has nothing to do with this issue. Please go ahead and file accordingly.