dansalvato / melee-gci-compiler

Melee GCI Compiler is an application and scripting language that makes it easy for Super Smash Bros. Melee mod developers to inject custom code and data into Melee save files.
Mozilla Public License 2.0
34 stars 9 forks source link

[Question] Optional refactoring #8

Open Nemris opened 1 year ago

Nemris commented 1 year ago

Hey there, Dan.

This might read a bit weird, but I'm in the habit of offering random refactorings to people so that I don't get too rusty with my Python.

I wanted to check if there's interest in PRs before going ahead, though. Still, one of the parts I thought about touching up is command-line args processing, e.g. by switching to argparse, which should improve that section's readability compared to the additional logic you have to use for getopt to give equivalent results.

dansalvato commented 1 year ago

Thanks for asking. I haven't really thought about it, but I'd say I'm probably not too open to "code cleanup" PRs, since it runs the risk of me defamiliarizing myself with my own codebase without any direct benefit to the end user. Of course, your suggestion is a very simple one and wouldn't cause that to happen, but for the sake of keeping contribution rules clear and consistent, I'll go ahead and say that those are better as issues rather than PRs. In any case, I appreciate your interest in the project, and we can keep this issue open for the suggestion of implementing argparse.

Nemris commented 1 year ago

Valid point for sure - and something that caused me to keep PRs from others on hold as well.

Regarding the benefits of argparse, I'd say some of the main ones would be automatic generation of the usage message based on the flags you define, plus an automatic -h/--help flag out of the box. It would also allow you to skip the whole pattern matching that you have in the main().

Shall I work on it in a fork and just link it here for discussing, for the time being?

Nemris commented 1 year ago

Alright, I went ahead and replaced getopt, as you can see here. While I tried to preserve most of the old behavior, you should be aware that the usage message is now slightly different, since argparse formats it in the style of *NIX utilities.

By the way, I noticed a quirk in your code which I'm not sure was intentional - apparently, you made it so that all of script_path, input_gci and output_gci can be omitted. In my variant, omitting those paths results in a run that appears simulated - in that it goes through the whole process but saves nothing on disk. In yours, the script simply dies without printing anything, possibly because of the catch-all at line 52.

As for sys, I saw that you're exiting with an uncommon code 10 if compiling fails, so I just kept it for the time being in case other tools need that status for compatibility.