feat: fallback front_female sprite to front_default for female-only p… - #1573
feat: fallback front_female sprite to front_default for female-only p…#1573programgames wants to merge 1 commit into
Conversation
379ce9e to
416c268
Compare
| serializer_class = PokemonDetailSerializer | ||
| list_serializer_class = PokemonSummarySerializer | ||
|
|
||
| def get_queryset(self): |
There was a problem hiding this comment.
Very interesting implementation. Can I ask what's the purpose of this function? It's for having ready at hand some data later on in the serializer.py part?
There was a problem hiding this comment.
Hi, @programgames caould you address the above question?
| "back_shiny_female": "back_shiny", | ||
| } | ||
|
|
||
| def _fill_female_sprites(self, node): |
There was a problem hiding this comment.
I'm strongly of the opinion that the data should just be shared with PokeAPI and consumers should handle this themselves.
There was a problem hiding this comment.
Among the years we always pulled data from veekun which in turn got it from decompilations. The data was sometimes pretty cryptic to use. I can mention the use of rare Unicode characters, having braces in strings that acted as templates, having hidden characters and so on. In the beginning we were reluctant to change these pieces of data turning to the users to implement custom strategies.
Eventually we cleaned the garbage that we were serving and everyone was happier. Maybe valuing user-friendliness over strict data rules was the key to it.
That said I'm in favour of these kind of changes. If a pokemon has a 100% female rate then the female_sprite can be inferred to be the same as the default_sprite. In this case default_sprite and female_sprite coincide. By having the female_sprite filled in (by this custom logic or any other method) is ok with me.
|
Free feel to ping me if changes are needed. |
| sprites = sprites_object.sprites | ||
| if obj.pokemon_species.gender_rate == 8: |
There was a problem hiding this comment.
@programgames would you be able to update your code and fix the conflict in this file? Recently @FallenDeity heavily rewrote it and I wasn't able to fix the conflict by myself.
…e-only pokemon For pokemon whose species has gender_rate=8 (exclusively female), fill the null female sprite keys with their default counterparts across all sprite sections. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5888657 to
457211b
Compare
| sprites: Any = sprites_list[0].sprites | ||
| if obj.pokemon_species is None or obj.pokemon_species.gender_rate != 8: | ||
| return sprites | ||
| if isinstance(sprites, str): |
There was a problem hiding this comment.
is it possible for sprites to be an str? don't we have it as a json field it should be auto parsed to a python dict
| if isinstance(sprites, str): | ||
| parsed = json.loads(sprites) | ||
| self._fill_female_sprites(parsed) | ||
| return json.dumps(parsed) |
There was a problem hiding this comment.
this will result in a json escaped string like {\"front_default just return a dict i dont think this if check is necessary
| if not sprites_list: | ||
| return {} | ||
| sprites: Any = sprites_list[0].sprites | ||
| if obj.pokemon_species is None or obj.pokemon_species.gender_rate != 8: |
There was a problem hiding this comment.
we can just do it like this imo
if obj.pokemon_species and obj.pokemon_species.gender_rate == 8:
self._fill_female_sprites(sprites)
return sprites| return sprites | ||
|
|
||
| def _fill_female_sprites(self, node: object) -> None: | ||
| if not isinstance(node, dict): |
There was a problem hiding this comment.
why guard against ourselves its expected that it be a dict here typehinting it object and just adding a separate guard seems like a bad idea just typehint it dict[str, Any] and instead move that if check to the for value in sprites.values() nested recursion loop to handle our nested sprite objects
saves us a cast call too
4 participants
Changes description
A simple solution for the #271 .
If the request if a simple GET for the pokemon, we select the specie and check if the gender rate is only female.
In this case we copy the valuer from default to female sprites with this mapping :
PS : I would like to know if Pokemon.objects.all().select_related("pokemon_species") could be a performance problem on the server, Should I test performance before and after this change to compare ?