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

Generate Foreign dex entries #324

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

h2o-DS
Copy link
Contributor

@h2o-DS h2o-DS commented Dec 28, 2024

Added foreign pokedex text to pokemon data files
Added foreign pokedex message files to tools/scripts/make_pokedex_message_banks.py
Renamed message banks 700-723

To be merged after #323

unk_020986CC -> pokedex_language
ov21_021D5600 -> pokedex_text
Added foreign pokedex text to pokemon data files
Added foreign pokedex message files to tools/scripts/make_pokedex_message_banks.py
Renamed message banks 700-723
Decided that diamond dex entries do not need to be generated
Moved pokedex data from data.json to pokedex.yaml
Seperated message banks that do not require pokedex data to make_species_message_banks.py
@@ -1,13 +1,16 @@
#!/usr/bin/env python3
import argparse
import json
import yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this pyyaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. How should I clarify that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's a new dependency, then we need to both clarify it in the INSTALL.md, provide a way to standardize what version we are pulling (e.g., via a requirements.txt for pip), and set up the Makefile to enforce virtual environments so that users don't have to worry about cross-pollinating project dependencies with their system packages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like apt has packaging for pyyaml by way of python3-yaml:

So we can add python3-yaml to our apt install command to ensure that a user has it on their system.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Scratch the above; Mac OS would be left in the dust with that approach. Homebrew has disabled the formula, and the official recommendation is to enforce virtual environments. So, we do need to move towards enforcing virtual environments via the Makefile.

@h2o-DS
Copy link
Contributor Author

h2o-DS commented Jan 2, 2025

pyyaml integration into INSTALL.md is dependent on changes in #329

@h2o-DS h2o-DS marked this pull request as draft January 2, 2025 19:03
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.

2 participants