Skip to content
This repository has been archived by the owner on Oct 2, 2023. It is now read-only.

Hisui #95

Merged
merged 18 commits into from
Jun 22, 2022
Merged

Hisui #95

merged 18 commits into from
Jun 22, 2022

Conversation

anhthang
Copy link

@anhthang anhthang commented Mar 2, 2022

I have added some new data for the new Pokemon and new forms introduced in Legends: Arceus

All of this data was collected manually from Bulbapedia (Japanese collected from Zukan)

Not sure what I'm doing is in the right way, or missing anything, so please help me verify if possible, then I can continue and correct it.

@Naramsim
Copy link
Member

Naramsim commented Mar 4, 2022

Hi @anhthang, sorry for the extremely late reply. Thanks infinitely for the additions. I'll have to review a bit the PR and understand how the new update works.

If I understood correctly this data is from the Arceus update, right? So we are still talking about gen 8?

@anhthang
Copy link
Author

anhthang commented Mar 5, 2022

Hi @Naramsim

Yes, this is gen 8 update from Arceus. So, please review it.

I also find out that some old pokemon has forms (cherrim, shellos, gastrodon, deerling, sawsbuck...), but they're missing pokemon/species id so it's missing in the API too. How we can add it safely? I thought it should be in a order like pokemon_id, is it?

@simonorono
Copy link

This is great @anhthang I noticed that the new Pokédex from Hisui, the addition of new Pokémon to the national dex and new moves/learn-sets is missing. Would that block the merging of this PR or can be contributed later? @Naramsim

I could help with those.

@anhthang
Copy link
Author

Thanks @simonorono. I'm still waiting feedback from @Naramsim before adding the others. New moves, TM/HM... maybe in another PR.

@Naramsim
Copy link
Member

I don't have any time to look into it guys. I'm sorry this has taken ages. Hopefully, I'll reserve some time and try to understand the new generation and the new data. But if there are other core members who are willing to review, then it might be way quicker.

@Naramsim
Copy link
Member

If you want we could test this data in our staging environment. Could you open an identical PR towards the staging branch of PokeAPI?

pokedex/data/csv/versions.csv Outdated Show resolved Hide resolved
@anhthang
Copy link
Author

Hi @Naramsim,

Versions & version groups are updated, pls verify again if you have time.

I also have a question, why some pokemon has forms but they don't have a pokemon_id (like Cherrim, Unown...)

@Naramsim
Copy link
Member

Hi @anhthang , thanks so much again! Imho, we can merge. @C-Garza any thoughts?

About the forms: there are forms and varieties. I forgot where the issue describing them is. I should pin it when I find it again.

@C-Garza
Copy link
Member

C-Garza commented May 17, 2022

Sorry, I haven't had time to look at this. I actually didn't notice this PR until you pinged me 😅. I can probably take a look at it tomorrow if you haven't already merged it by then.

@anhthang
Copy link
Author

just fixed the flavor texts, break lines and sort

@anhthang anhthang marked this pull request as ready for review May 20, 2022 02:44
Comment on lines 123136 to 123141
484,39,9,"This Pokémon is feared as a deity in Hisuian legend. The birth
of Palkia was what caused the walls of our world to disappear,
creating a sky that spans for infinity."
484,39,9,"It soars across the sky in a form that greatly resembles the
creator of all things. Perhaps this imitation of appearance is
Palkia's strategy for gaining Arceus's powers."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see two flavor texts for the same entry. I'm assuming this pokemon has an additional entry for a form or something. I remember there being an issue about this before and the way it was resolved was for now was removing the form's entry. Same thing for id 483 and 905.
Found the old issue

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing that. I have removed just now.

@C-Garza
Copy link
Member

C-Garza commented May 23, 2022

I left one comment, other than that it looks good to me.

@Naramsim
Copy link
Member

Nice, I'll try build pokeapi with this data later

Naramsim added a commit to PokeAPI/pokeapi that referenced this pull request May 24, 2022
@@ -897,3 +897,10 @@ id,identifier,generation_id,evolves_from_species_id,evolution_chain_id,color_id,
896,glastrier,8,,474,9,8,,-1,3,35,0,120,0,1,0,1,0,896,
897,spectrier,8,,475,1,8,,-1,3,35,0,120,0,1,0,1,0,897,
898,calyrex,8,,476,5,12,,-1,3,100,0,120,0,1,0,1,0,898,
899,wyrdeer,8,234,,4,,,4,,,0,,,1,,0,0,899,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, here the data lacks information. By building the DB I get that:

null value in column "forms_switchable" of relation "pokemon_v2_pokemonspecies" violates not-null constraint

I guess we should try to fill all the columns.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Naramsim updated in last commit

Copy link
Member

@Naramsim Naramsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice thanks!

@@ -897,3 +897,10 @@ id,identifier,generation_id,evolves_from_species_id,evolution_chain_id,color_id,
896,glastrier,8,,474,9,8,,-1,3,35,0,120,0,1,0,1,0,896,
897,spectrier,8,,475,1,8,,-1,3,35,0,120,0,1,0,1,0,897,
898,calyrex,8,,476,5,12,,-1,3,100,0,120,0,1,0,1,0,898,
899,wyrdeer,8,234,,4,,,4,,,0,,,1,,0,0,899,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naramsim added a commit to PokeAPI/pokeapi that referenced this pull request May 25, 2022
@Naramsim
Copy link
Member

Yay! It's working!

@Naramsim
Copy link
Member

Naramsim commented Jun 22, 2022

@anhthang
Copy link
Author

@Naramsim I just fixed a small bug. I think the other is fine to me

@Naramsim Naramsim merged commit 3530bc7 into PokeAPI:master-pokeapi Jun 22, 2022
Naramsim added a commit to PokeAPI/pokeapi that referenced this pull request Jun 22, 2022
@Naramsim
Copy link
Member

@anhthang, it's live! Thank you so much for the effort in these months. I know I took quite a while, but we maintainers don't have much time anymore.

Thanks again!

@anhthang
Copy link
Author

@Naramsim thank you for your support 💪 can you update graphql also? it seems not updated

@Naramsim
Copy link
Member

Ahhh, hahah yeahh, I'll try!!!

@Naramsim
Copy link
Member

Naramsim commented Jul 6, 2022

I've updated the graphql, can you check?

@anhthang
Copy link
Author

anhthang commented Jul 6, 2022

I've updated the graphql, can you check?

Just tested, it seems working fine except some optional data. I will work on it later if I have time

@bdawg1989
Copy link

Was this ever merged and fixed? All of the Hisui mons don't have gen 8 learnsets or moves that I can pull.

@Naramsim
Copy link
Member

Yes this was merged

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

Successfully merging this pull request may close these issues.

5 participants