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

[Question] Optional refactoring #8

Open
Nemris opened this issue Jun 8, 2023 · 3 comments
Open

[Question] Optional refactoring #8

Nemris opened this issue Jun 8, 2023 · 3 comments

Comments

@Nemris
Copy link

Nemris commented Jun 8, 2023

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
Copy link
Owner

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
Copy link
Author

Nemris commented Jun 9, 2023

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
Copy link
Author

Nemris commented Jun 9, 2023

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.

@Nemris Nemris mentioned this issue Jun 21, 2023
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

No branches or pull requests

2 participants