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

Updating MoveDetailSerializer to change $effect_chance to actual number #1031

Merged
merged 10 commits into from Feb 19, 2024

Conversation

SKCwillie
Copy link
Contributor

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.

@SKCwillie
Copy link
Contributor Author

Sorry for all the commits. I keep finding issues or small things that can make the code a little cleaner.

@GreatNovaDragon
Copy link
Contributor

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.

@SKCwillie
Copy link
Contributor Author

SKCwillie commented Feb 7, 2024

@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.

@Naramsim
Copy link
Member

Naramsim commented Feb 7, 2024

@SKCwillie thanks for trying and figuring out how the code works :)

Could you please fix the linting issues? Running black .. Afterwards I can deploy in staging

@SKCwillie
Copy link
Contributor Author

@Naramsim checks are passing now!

Copy link
Contributor

@simonorono simonorono left a 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.

Comment on lines 2346 to 2351
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}"
)
Copy link
Contributor

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}"
            )

Copy link
Contributor Author

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

@Naramsim
Copy link
Member

In the meantime I deployed the current version to the staging site:

https://staging.pokeapi.co/api/v2/move/fire-punch
https://pokeapi.co/api/v2/move/fire-punch

https://staging.pokeapi.co/api/v2/move/dire-claw
https://pokeapi.co/api/v2/move/dire-claw

But actually what @simonorono says is of great value. I forgot we also have the GQL 😅

@simonorono
Copy link
Contributor

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.

@Naramsim
Copy link
Member

Naramsim commented Feb 10, 2024

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:

  • Merging the PR with Simon's fixes and possibly having the GraphQL API only show $effect_chance%.
  • Creating and running a tool that beforehand updates the static CSV. This has to be well thought out though.
  • Don't do anything. Just manually correct the effect entries and remember that when new data comes in also the effect description of the move has to change.
  • studying Postgres/Django/Hasura and checking if they have some ways of referencing data from other tables inside a string text. maybe this or running two queries, one for fetching the text and one for fetching the effect value, then merging the results. Or even using Views

@simonorono
Copy link
Contributor

Another solution could be having the build step generate effect entries for every different value of move_effect_chance

image

@Naramsim
Copy link
Member

Another solution could be having the build step generate effect entries for every different value of move_effect_chance

I don't follow you very well on this but I'm lacking SQL/Django knowledge at the moment.

@Naramsim Naramsim merged commit b5d617a into PokeAPI:master Feb 19, 2024
3 checks passed
@pokeapi-machine-user
Copy link

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 staging branch will be deployed in our staging environment and the entire API will be ready to review.

A Pull Request (master<-staging) will be also created at PokeAPI/api-data and assigned to the PokeAPI Core team to be reviewed. If approved and merged new data will soon be available worldwide at pokeapi.co.

@pokeapi-machine-user
Copy link

The updater script has finished its job and has now opened a Pull Request towards PokeAPI/api-data with the updated data.

The Pull Request can be seen deployed in our staging environment when CircleCI deploy will be finished (check the start time of the last build).

Naramsim pushed a commit to PokeAPI/api-data that referenced this pull request Feb 21, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request Feb 26, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request Mar 4, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request Mar 11, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request Mar 18, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request Mar 25, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request Apr 1, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request Apr 8, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request Apr 15, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request Apr 22, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request Apr 29, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request May 6, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request May 13, 2024
pokeapi-machine-user added a commit to PokeAPI/api-data that referenced this pull request May 20, 2024
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

Successfully merging this pull request may close these issues.

Replace $effect_chance% with actual value
5 participants