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

Name asset extraction tools more consistently #2316

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cadmic
Copy link
Contributor

@cadmic cadmic commented Dec 1, 2024

Now they're all run as $(PYTHON) tools/extract_foo.py.

@hensldm
Copy link
Contributor

hensldm commented Dec 1, 2024

For the text, I like doing something similar to audio. Have the two scripts in a folder called text, than create an extract_text.py outside it that parses the config for what the text extract script needs, and calls msgdis's main function (in zeldaret/mm#1750 I renamed it to extract).

@cadmic
Copy link
Contributor Author

cadmic commented Dec 1, 2024

hm, I think that could make sense if the main function was identical to MM's like with the audio tools. Otherwise, I think the extra indirection is not worth it.

Either way, could we consider that out-of-scope for this PR?

@hensldm
Copy link
Contributor

hensldm commented Dec 1, 2024

The main function in msgdis.py should be the same.
Personally I don't think it is out of scope, mainly so we don't have to rename msgdis.py to extract_text.py and than rename it back to msgdis.py

@cadmic
Copy link
Contributor Author

cadmic commented Dec 1, 2024

The main function in msgdis.py should be the same.

hm, I tried a simple diff and it didn't show that many similarities (and in MM it seems to be split into nes/msgdisNES.py and staff/msgdisStaff.py too)

mainly so we don't have to rename msgdis.py to extract_text.py and than rename it back to msgdis.py

Why's that? I don't think an extra commit would cost that much

@cadmic
Copy link
Contributor Author

cadmic commented Dec 1, 2024

are you concerned about losing history due to git shenanigans if we rename it later and aren't careful?

@hensldm
Copy link
Contributor

hensldm commented Dec 1, 2024

hm, I tried a simple diff and it didn't show that many similarities (and in MM it seems to be split into nes/msgdisNES.py and staff/msgdisStaff.py too)

Think you are outdated. Updated version should just have msgdis.py and msgenc.py in tools/text

Why's that? I don't think an extra commit would cost that much

More so personal preference of keeping the git history cleaner.
However if other people have the same opinion that this is out of scope, I won't hold it back.

@cadmic
Copy link
Contributor Author

cadmic commented Dec 1, 2024

Think you are outdated. Updated version should just have msgdis.py and msgenc.py in tools/text

Ah yeah, my bad. It still seems like a bit of work to factor out all of the differences though

More so personal preference of keeping the git history cleaner.

I'm also a fan of a clean git history, although my idea of that is:

  1. Every change builds (makes bisecting and merges easier)
  2. Every change has a clear commit message
  3. Every change is as self-contained as possible and only does one thing

Number 3 there is often at odds with minimizing the total number of commits. For example, I think this change would be better as "name asset extraction tools more consistently" instead of "name asset extraction tools more consistently, and also prepare for unifying OOT and MM text extraction".

(As an aside, in the same vein I much prefer a bunch of 1-line commits rather than "Misc Cleanup", since mixing changes together like that makes it harder to read the history.)

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

Successfully merging this pull request may close these issues.

3 participants