Added a way to get encounters per Pokémon

[GeoffreyFrogeye]

The function is called get_location_area to stay consistent with the API but this is a bit weird. If it were up to me I'd swap PokemonEncounter and LocationAreaEncounter so the name matches the anchor point of the encounter and not the attributes inside. What do you think?

I also added a always_list attribute so it stills give a list even if there's only one encounter location. This might be rendered obsolete with a proper pagination implementation though.

Pagination implementation will also render obsolete testing done for this feature so I didn't really put much thought in it. Furthermore, I didn't really understood the way the tests were made (it only checks a string?).

I couldn't test this on others versions of Python because tox was throwing an error I gave up trying to fix. Sorry about that.

[codecov[bot]]

Codecov Report

Merging #44 into master will decrease coverage by 0.08%. The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
- Coverage   99.81%   99.72%   -0.09%     
==========================================
  Files           3        3              
  Lines        1099     1105       +6     
==========================================
+ Hits         1097     1102       +5     
- Misses          2        3       +1

Impacted Files	Coverage Δ
pokepy/api.py	98.26% <100%> (+0.03%)	:arrow_up:
pokepy/resources_v2.py	99.89% <75%> (-0.11%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9593c83...4567edc. Read the comment docs.

[Naramsim]

Thanks for the contributions! I like the addition of listing the encounters.

Shouldn't a list always be returned? So we are always consistent. What do you think?

[GeoffreyFrogeye]

Shouldn't a list always be returned? So we are always consistent. What do you think?

Do you mean for the whole API (all the get_ calls)? I would be in favor of it, but it's a big breaking change, and I don't know if the userbase will like it.

If you meant just for get_location_area, it's already the case. I made sure to always return a list.

[Naramsim]

Just for get_location_area. So why did you add the always_list parameter?

[Kronopt]

Hey GeoffreyFrogeye, thanks for the PR!

You just made me realize that the location_area_encounters attribute of the pokemon resource is just a string with a link. @Naramsim @tdmalone why isn't this a list of NamedAPIResource or APIResource of Location Areas?

This question aside, I've started to work on hypermedia resources a while ago, which would handle attributes linking to other resources. So location_area_encounters should be covered once that is finished, I just haven't had the time to further test what I've written so far. You can check it out on the pagination branch.

In short, I think you did well with this PR, I just think it will become obsolete when hypermedia resources handling is correctly implemented.

[Kronopt]

Other comments:

If it were up to me I'd swap PokemonEncounter and LocationAreaEncounter so the name matches the anchor point of the encounter and not the attributes inside. What do you think?

You mean swapping the names of the PokemonEncounter attribute of LocationArea resource with LocationAreaEncounter attibute of Pokemon?

I didn't really understood the way the tests were made (it only checks a string?).

The tests check if all get_ methods correctly parse the JSON response of the PokéAPI. To do that a string comparison is done to verify that the correct value is assigned to the generated python method.

[GeoffreyFrogeye]

So why did you add the always_list parameter?

Because the current behavior is to return a list only if its length is two or more.

When always_list is True, it makes sure that the return a list, even if the length of the list (as given by Beckett) is one.

When always_list is False or undefined (the default), it uses the current behavior : if the length of the list (as given by Beckett) is one (which is the most used case, e.g. get_pokemon('snivy')).

Maybe the isinstance(final, list) is confusing, since as far as I tested final was always a list, even for calls when you give an uid.

In short, I think you did well with this PR, I just think it will become obsolete when hypermedia resources handling is correctly implemented.

It's even better if it's replaced by something clean! This was just a quick modification for me, I did not expect this submission to be long-lived.

You mean swapping the names of the PokemonEncounter attribute of LocationArea resource with LocationAreaEncounter attibute of Pokemon?

Yes. I find it confusing, having worked with APIs that use the opposite convention. I guess it should be discussed on the API project, not here. I haven't looked into that yet.

[Kronopt]

Because the current behavior is to return a list only if its length is two or more.

This is intended behaviour as the majority of PokéAPI resources and subresources return just one object, so having a list with a single element didn't make much sense. For those cases where more than one object is returned, then yes, they are returned in a list. In this case, you wanted the result to always be a list. That's fair, and probably makes sense.

Maybe the isinstance(final, list) is confusing, since as far as I tested final was always a list, even for calls when you give an uid.

True. I don't think that check is needed. I went through the beckett code and it checks out. I ran the tests without it and it works ok. If you want, you can open a PR with that change and I'll merge it

Yes. I find it confusing, having worked with APIs that use the opposite convention. I guess it should be discussed on the API project, not here. I haven't looked into that yet.

This should be discussed on the pokeapi repo, yes

[GeoffreyFrogeye]

If you want, you can open a PR with that change and I'll merge it

I did, and also merged it here just in case you'd want to merge this before merging pagination. If not, feel free to close this PR.

This should be discussed on the pokeapi repo, yes

Honestly this is so minor that I don't want to bother people with that. I finally got used to it.

[Kronopt]

I'll leave this open for now

[Naramsim]

You just made me realize that the location_area_encounters attribute of the pokemon resource is just a string with a link. @Naramsim @tdmalone why isn't this a list of NamedAPIResource or APIResource of Location Areas?

Hi, basically because we needed a very quick solution to address a big problem. At that time the API was overloaded. All the requests to retrieve the /pokemon/id endpoint were taking so long. So the API was almost unusable because it was slow (at that time we didn't serve static cached files).

I guess that we could go back now.