Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flavor Texts for different forms/varieties all mixed together #533

Closed
jonduenas opened this issue Oct 17, 2020 · 19 comments
Closed

Flavor Texts for different forms/varieties all mixed together #533

jonduenas opened this issue Oct 17, 2020 · 19 comments

Comments

@jonduenas
Copy link

This may be a feature request rather than a bug report. I guess it depends on what the intention is behind the current implementation.

I'm trying to parse the flavor text for a given Pokemon species. This is easy for Pokemon that only have one variety or form. But if I want to show the flavor text for a Pokemon with various forms or varieties, they're all mixed up for the same species and there doesn't seem to be any way to distinguish which is which.

For example, Raichu. When I download it all from the API, there doesn’t seem to be any way to tell the difference between the entry for Alolan Raichu and Kanto Raichu. And I may be wrong, but it looks like they’re not even ordered consistently. I think for lets-go-pikachu/lets-go-eevee the Alola entry is first, but for sword/shield the Kanto entry is first. Looking at Meowth is even more confusing because of Kanto, Alola, Galar, and Gigantamax are all there and the order is inconsistent even between sword and shield.

If this isn't intended, then consider this a bug report. If this is intended, then consider this a feature request that somehow these varieties of flavor texts are somehow separated or at least sorted consistently so when it's parsed out it's at least easy to know which is which. Hopefully I'm not just missing some other organizational system for this? If so, then consider this just a question on how to sort through this.

@jgarrow
Copy link

jgarrow commented Oct 18, 2020

I noticed the same issue as well. I cloned the repo to make my own SQLite3 database. Looking through the tables, it seems that the flavor text entries are tied to a pokemon species id instead of a pokemon id. There’s no way to tell which flavor text belongs to which species variant. For my own server, my workaround was to add a new pokemon_id column to the flavor text table. Would be nice if a similar change was made on the official API

@jonduenas
Copy link
Author

Thanks for the extra context @jgarrow. I wonder if this is potentially an issue sourced from the original Veekun database? A lot of this backend stuff is outside my realm of knowledge, so I'm just guessing.

@jgarrow
Copy link

jgarrow commented Oct 19, 2020

I imagine so since the database is built using the CSV files. But I haven't looked at the CSV's themselves, so I'm not positive

@phalt
Copy link
Member

phalt commented Oct 20, 2020

Most of the content comes from the CSV's from Veekun's, yes, but we're slowly adding in Generation 8 data in our own fork. I suspect that if data is not present there, then it is not in our database. We'd welcome changes to the data if it makes sense!

@C-Garza
Copy link
Member

C-Garza commented Oct 20, 2020

This issue is from adding new Gen 7 and Gen 8 entries from the PokeAPI fork of Veekun's repo. I don't think past generations had to deal with multiple pokedex entries for different pokemon forms, so the pokemon species id was all that was needed.

For clarity, an example of the issue can be seen in the pokemon_species_flavor_text.csv at rows 3034 and 3035, two different Raichu pokedex descriptions for the same version where one (3034) is the Alolan form.

I think adding another column like pokemon id could work, but @Naramsim has mentioned not adding new columns in #520, but I'm thinking we might have to.

@phalt
Copy link
Member

phalt commented Oct 20, 2020

Yeah the problem is API compatibility. If we add a new field to the API format it could break thousands of integrations.

@jgarrow
Copy link

jgarrow commented Oct 20, 2020

What kind of integrations would need to get changed? I would think that mutating or deleting existing columns would for sure break things. But adding a new column shouldn't break existing integrations, right? This seems like an issue that's going to grow as Galarian and Gigantamax form pokedex descriptions get added as well...I'd love to help out with it. What direction or next steps would need to be taken next? I'm not familiar enough with the CSV files and ins and outs to jump right in

@phalt
Copy link
Member

phalt commented Oct 21, 2020

Third party integrations that use this service, we can't guarantee all those projects handle the API responses in the same way, so I'm always wary about adding new fields. New columns means new API fields.

@zihadmahiuddin
Copy link

But adding a new column shouldn't break existing integrations, right?

I think so, but in some cases, like if the field is added in the middle and not in the end, it could cause problems.
For example, we download the raw CSVs and convert them to JSON, and then use the local JSON files in our project. But this commit inserted 2 new fields before order which was a breaking change for us as order became is_legendary etc. But thankfully we had tests that were failing because of this and were able to fix it fast. But if new fields are added without touching the old ones, I don't think that would be a breaking change.

@DDriggs00
Copy link
Contributor

This is a pretty severe issue. It basically invalidates the flavor text of any Pokemon with multiple forms.

@DDriggs00
Copy link
Contributor

DDriggs00 commented Nov 14, 2021

Perhaps this could be solved by copying this table to pokemon_flavor_text or pokemon_form_flavor_text, and replacing species_id with pokemon_id or form_id, then either deprecating this table or removing non-species-primary rows.

@gruxor
Copy link

gruxor commented Dec 7, 2021

This is a pretty severe issue. It basically invalidates the flavor text of any Pokemon with multiple forms.

Definitely.

Third party integrations that use this service, we can't guarantee all those projects handle the API responses in the same way, so I'm always wary about adding new fields. New columns means new API fields.

If any of those existing projects use the PokeDex descriptions they have to parse them manually (which defeats the purpose of the API in the first place), and this is also an issue for all new projects that try to use the text as well.

It's not my place to say one way or the other obviously, but if I had the choice between breaking some old projects (who can update, likely with little effort) or having an entire section of the API be indefinitely dysfunctional (over a year and counting!), I'd definitely go with the former.

Naramsim added a commit that referenced this issue Dec 7, 2021
@Naramsim
Copy link
Member

Naramsim commented Dec 7, 2021

I think adding another column like pokemon id could work, but @Naramsim has mentioned not adding new columns in #520, but I'm thinking we might have to.

Hello @C-Garza, I approach this issue for the first time. So, apparently, Veekun modified the file we are speaking about removing this inconsistency.

Luckily enough their species_ids and version_ids are the same as ours so we can just replace the file. I opened a PR: #676

@Naramsim
Copy link
Member

Naramsim commented Dec 7, 2021

Can somebody test if the new version works well? -> https://staging.pokeapi.co/api/v2/pokemon-species/raichu

@gruxor
Copy link

gruxor commented Dec 7, 2021

Can somebody test if the new version works well? -> https://staging.pokeapi.co/api/v2/pokemon-species/raichu

It's a definite improvement because there aren't identically-tagged entries now, but from a quick ctrl+F it looks like the Alolan dex description isn't anywhere in there. Is the intent to move the regional flavor text into the variant files? (that would make sense to me, but I'm not sure if that presents issues for the API's overarching structure. if that's the plan, it seems to me that you'd be better off divorcing the flavor text from species and putting it solely into the /pokemon/ directory).

Also, it seems the "form_descriptions" object is empty, could (obviously) be related if that's the intended destination for the regional form text. Or maybe the species object had that before and I'm just forgetting.

@C-Garza
Copy link
Member

C-Garza commented Dec 7, 2021

Yeah that should fix the identically tagged entries. This became an issue when we merged this PR from the Veekun fork. Veekun doesn't have a way to deal with the different form pokedex descriptions so they haven't added them. There's an open issue over at Veekun.

@Naramsim
Copy link
Member

Naramsim commented Dec 7, 2021

Ok, I say then to merge this PR and fix the issue. We will figure out later what to do with the missing form flavor text.

@Naramsim
Copy link
Member

Naramsim commented Dec 9, 2021

I guess that now this issue is resolved as well on the live version of pokeapi. Can anyone confirm?

@C-Garza
Copy link
Member

C-Garza commented Dec 10, 2021

I don't see identically tagged entries for raichu anymore so I'm assuming it worked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants