-
-
Notifications
You must be signed in to change notification settings - Fork 955
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
Updating MoveDetailSerializer to change $effect_chance to actual number #1031
Conversation
Sorry for all the commits. I keep finding issues or small things that can make the code a little cleaner. |
i've got a question: is that the text that is displayed via the api, or the internally saved text that is changed here? (I dont know how pokeapi works internally) I dont think the internal text should be changed, only the displayed text. |
@GreatNovaDragon because the proposed change is taking place in the serializers file, the sql.lite database is already built, so no data is getting changed. I'm basically Ctrl F and replacing the $effect_chance in the payload at the last possible moment. |
@SKCwillie thanks for trying and figuring out how the code works :) Could you please fix the linting issues? Running |
@Naramsim checks are passing now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great for the API, however this won't translate to the GraphQL implementation, for that we would need to do this while building the data.
pokemon_v2/serializers.py
Outdated
effect_entries = data[0] | ||
for _i, k in enumerate(effect_entries): | ||
if "$effect_chance%" in effect_entries[k]: | ||
data[0][k] = effect_entries[k].replace( | ||
"$effect_chance", f"{obj.move_effect_chance}" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code fails for moves without effect entry like dire claw or hard press (both introduced in gen 9).
It could also be written without needing extra or unused variables; here are my suggested changes:
if len(data) > 0:
for key, value in data[0].items():
if "$effect_chance%" in value:
data[0][key] = value.replace(
"$effect_chance", f"{obj.move_effect_chance}"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed your suggested changes. Definitely a good catch on moves without effect entries
In the meantime I deployed the current version to the staging site: https://staging.pokeapi.co/api/v2/move/fire-punch https://staging.pokeapi.co/api/v2/move/dire-claw But actually what @simonorono says is of great value. I forgot we also have the GQL 😅 |
I just realized that it's not possible to do this during the build of the database since several moves share the same effect entry, e.g. fire punch, lava plume, inferno, etc. all share the same effect entry. |
So PokeAPI's Postgres has links for different moves to the same effect entry and it's all dynamic. Correct? Yeah, just checked. So the build step is off-limits. I see some routes:
|
I don't follow you very well on this but I'm lacking SQL/Django knowledge at the moment. |
A PokeAPI/api-data refresh has started. In ~45 minutes the staging branch of PokeAPI/api-data will be pushed with the new generated data. |
The updater script has finished its job and has now opened a Pull Request towards PokeAPI/api-data with the updated data. |
resolve #1024
Take effects text from the seralizer and loop over dictionary looking for "effect_chance" when it is found, replace it with move_effect_chance.
I know the build file was mentioned in the comments of the issue but I don't see a way to edit the build that can be dynamically edited later. It definitely could be possible but will have to be done b someone with more knowledge on the build file than me.
To test this, it can be deployed to Staging and the endpoints can be hit to confirm changes.