Repository navigation
Conversation
765d38a to
9c995cc
Compare
FallenDeity
left a comment
There was a problem hiding this comment.
also add tests for the new models/serializers etc
ps: a rebase is also needed
| 16,245,10057,1 | ||
| 16,246,361,0 | ||
| 16,247,362,0 | ||
| 16,247,10074,0 |
There was a problem hiding this comment.
should order be 0 for both base glalie and mega? similar for eggxecutor its 1 for both normal and alola can u check
| build_generic((PokemonDexNumber,), "pokemon_dex_numbers.csv", csv_record_to_objects) | ||
|
|
||
| def csv_record_to_objects(info): | ||
| dex_number = PokemonDexNumber.objects.get( |
There was a problem hiding this comment.
should we use something like this to pre load a look up dict? PokemonDexNumber.objects.all() rather than multiple queries
There was a problem hiding this comment.
I'm not sure since it is not done anywhere else in the file, and I don't know if it is pertinent since all we really need for an entry is the corresponding PokemonDexNumber
There was a problem hiding this comment.
well currently it would execute one query everytime the fn is called its additional overhead once we add more games having a pre loaded dict on top level to fetch as a lookup cache would be much more efficient, i agree its not done elsewhere but thats because build.py is inefficient it does need a refactor :)
| @@ -0,0 +1,367 @@ | |||
| pokedex_id,pokedex_number,pokemon_id,order | |||
There was a problem hiding this comment.
They are all of the Sun/Moon/Ultra Sun/Ultra Moon Pokédexes, since they were divided by islnd and USUM are different from SM in their entries.
We will have one file, but yes, the entries will need to be duplicated on all these Pokédexes, and each of the other ones existing in the API, from the national and Kanto dexes all the way to the Z-A and Champions dexes.
| @extend_schema_field(PokemonDexNumberSerializer(many=True)) | ||
| def get_pokedex_entries(self, obj: Pokedex) -> ReturnList[ReturnDict[str, Any]]: | ||
| entries = ( | ||
| PokemonDexNumber.objects.filter(pokedex=obj) |
There was a problem hiding this comment.
we might need to prefetch the varieties here
There was a problem hiding this comment.
I'm sorry I do not quite understand how to do this
There was a problem hiding this comment.
we pre fetch pokemon species here we can also use prefetch_related to prefetch the varieties, basically instead of querying each variety present in a dex as individual query it would collapse it to a single query so overall 2 queries give it a read u might find some other blog posts online too https://www.compilenrun.com/docs/framework/django/django-performance/django-prefetch-related/
| 16,201,170,0 | ||
| 16,202,171,0 | ||
| 16,203,772,0 | ||
| 16,204,774,0 |
|
also i think this pr needs to be expanded a bit currently this pr would tell for pokedex whether its present in that dex but it dosent cover games we already have a mapping which i think we can extend https://github.com/PokeAPI/pokeapi/blob/master/data/v2/csv/pokemon_game_indices.csv we can add entries per mon available in game here with similar data we do for pokedex i have some exclusion lists which can be used for automated population https://github.com/PokeAPI/sprites/blob/0dc105d5b12416932f29b594078282ab4be045f5/scripts/tracker/common.py#L173-L416 also some additional logic which can be used to auto generate the whole csv https://github.com/PokeAPI/sprites/blob/master/scripts/tracker/build_sprite_index.py currently its unused for example this along with a new field in version-groups endpoint is what i had in mind since after dexit its not guranteed all pokemon are in the pokedex right lot of mons are transferable etc |
The idea of this PR was to have the exact order of the varieties in the Pokédex, to be able to say that for Alola Pokédex, Alolan Rattata is the first one to be shown in the Pokédex. I don't think your solution has this system after seeing it.
If a Pokemon is not in the game's Pokédex, it will not appear at all for the Pokédex, so even if it is transferable, the varieties will not appear, this behaviour will not change I am open to all kinds of suggestions to make this better, I mainly took into account what was said in the #1266 on what to do as a base but maybe we can have something easier to do |
|
just to add i am not saying my solution is different from urs what change u made will still be there what i suggested will just build on top of it, so to clarify along with your change we can also have entries in the pokemon game indices which we already do have the mapping only thing missing is the data so basically the data ur right now generating for varities we can also map to game indices than just pokedex its 2 different problems both solve
|


Change description
This PR's purpose is to add Pokémon varieties inside the Pokédex (for example, in Alola Pokédex, Rattata has both his alolan and kantonian forms). It is linked to this issue #1266
Currently, the only Pokédex impacted by the change is the original-alola Pokédex (ID 16). It is fully implemented for this Pokédex and since the code has been updated with all the required models, next changes should be faster.
What to expect for the new data ? Here is an example of Rattata's entry in the original-alola Pokédex :
{ "entry_number": 15, "pokemon_species": { "name": "rattata", "url": "http://localhost:8000/api/v2/pokemon-species/19/" }, "varieties": [ { "pokemon": { "name": "rattata-alola", "url": "http://localhost:8000/api/v2/pokemon/10091/" }, "order": 0 }, { "pokemon": { "name": "rattata", "url": "http://localhost:8000/api/v2/pokemon/19/" }, "order": 1 } ] },The order is the same order at the one seen on Bulbapedia's page : https://bulbapedia.bulbagarden.net/wiki/List_of_Pok%C3%A9mon_by_Alola_Pok%C3%A9dex_number_in_Pok%C3%A9mon_Sun_and_Moon
AI coding assistance disclosure
No AI was used
Contributor check list