Add cli package, allow pack music from command line, not require qt anymore. #105
Conversation
There was a problem hiding this comment.
I think this looks pretty good, but I did see some things that should be changed before merging. My comments that start with "Fix:" need to be fixed before I will merge this pull request; comments that start with "Recommend:" don't necessarily need to be fixed but it would be great if you could fix at least some of them. If you disagree with any of my comments I think you can reply to them to start a conversation.
2 suggestions:
- Add more comments, especially comments explaining why you did what you did. It's easy to see how everything works but the why is sometimes not as clear.
- Try not to redefine constants that are already defined for you. If you can pull some values from
src.definitionsit will be much easier to maintain this code.
There was a problem hiding this comment.
What's the intended way to run the CLI? I can copy this file to the top level and run it with python, but that doesn't feel like the way you intended it to work.
There was a problem hiding this comment.
Also, is the CLI application completely separate from the GUI app? Like if I run the GUI app it won't run any of the CLI code and vice versa?
There was a problem hiding this comment.
The intended way to run the CLI is by using it as a Python package. When you're in the root directory of the 'infinite-music-discs' folder, you can run a command like python -m src.cli -g 1.20.2 -f TestMusicEntries.csv -d C:\temp\test_autobuild_output -c to invoke the CLI.
The CLI program depends on modules from the GUI program to function properly. The CLI program relies on the factory class in the src.generator module, which you defined in the original GUI program. However, when the GUI program is running, it doesn't require any code from the CLI package.
I suggest that we collaborate to abstract the code further by separating the GUI part from the core program logic. This will ensure better maintainability. I know the current code abstraction is quite good -- you can start the whole process by passing parameters to the GeneratePackWorker. However, some adjustments may be needed in terms of code organization and package structure. For example, you could consider splitting the code into three packages: core, cli, and gui.
The way you've currently distributed this Python program isn't quite standard. I recommend using setuptools to package and publish the amazing project to PyPi. By using the setuptools, you can define entry points in your program, which precisely control which Python function a binary command runs, making it easier to run and distribute your program.
Thank you for your great work, hope we can work together to make it better.
| setting = cli_parser.get_settings() | ||
| chdir(cli_parser.output_dir) | ||
| worker = HeadlessGeneratePackWorker(musics, setting) | ||
| worker.run() |
There was a problem hiding this comment.
Fix: call worker.generate() instead of worker.run() to be consistent with the QT-based worker
| from src.cli.types import DPRPVerPair, Settings | ||
| from src.definitions import PackFormatsDict, DiscListEntryContents, DiscListContents | ||
|
|
||
| from src.cli.logger import * |
There was a problem hiding this comment.
Fix: IIRC import * is bad practice. Explicitly import what you need from your logger module, and consider designing the logger module with importability in mind. Or you could expose a setup() function in the logger module and do something like:
import src.cli.logger as cli_logger
cli_logger.setup()
| # package_name='infinite_music_discs', dest_dir='.', compress_output=False) | ||
|
|
||
| def __init__(self): | ||
| parser = ArgumentParser(prog="mc music packer") |
There was a problem hiding this comment.
Fix: program should be named "imd" or "infinite-music-discs" or something similar, to associate it with the project name
| help="a csv file delimited by semicolon, " | ||
| "each line of which contains texture_file;track_file;title;internal_name.") | ||
| parser.add_argument("-g", "--minecraft-version", type=str, required=True, | ||
| help="specify a game version, support 1.14 - 1.20.2", |
There was a problem hiding this comment.
Fix: import PackFormatsDict from src.definitions and parse it to determine the range of supported game versions. Otherwise, someone has to remember to come here and update the help message whenever a new version is added.
|
|
||
| def is_legacy_version(mc_ver: str) -> bool: | ||
| mc_ver_val: list[int] = [int(i) for i in mc_ver.split(".")] | ||
| return mc_ver_val <= [1, 19, 3] |
There was a problem hiding this comment.
Recommend: Reference LEGACY_DP_LATEST_VERSION in src.definitions instead of hardcoding 1.19.3 here. You can compare that pre-defined legacy datapack version against the datapack pack_format you get from get_dp_rp_version
| parser.add_argument("-g", "--minecraft-version", type=str, required=True, | ||
| help="specify a game version, support 1.14 - 1.20.2", | ||
| default="1.20.2") | ||
| parser.add_argument("-p", "--use-parallel", action='store_true', |
There was a problem hiding this comment.
Fix: use_parallel implies that the user should set this flag if they want to use the parallel processing, but this flag is connected to the skip_proc setting which will skip the parallel processing if it's set to True. This flag should be renamed to something like proc-serially or proc-sequential so the user knows that setting it will disable the parallel processing.
| @@ -0,0 +1,11 @@ | |||
| from src.cli.cli_parser import CLIParser | |||
There was a problem hiding this comment.
Fix: copy the shebang from main.pyw so you can run the CLI directly:
#!/usr/bin/env python3
| self._progress += 1 | ||
|
|
||
|
|
||
| def generate(self): |
There was a problem hiding this comment.
Recommend: Catch IMDException here and sample its status (which will be of type Status from src.definitions) to print the appropriate helpful message from StatusMessageDict so the user isn't guessing what a particular custom exception means. If you want, you could also reraise the IMDException you caught to crash the program afterward so the user gets a stack trace.
| help="package name") | ||
| parser.add_argument("-o", "--output-dir", type=str, default=".", help="the output directory of rp&dp.") | ||
| parser.add_argument("-z", "--compress-output", action='store_true', help="compress output pack to zip") | ||
| parser.add_argument("-c", "--cover-image", type=str, default='', help="cover image of the pack") |
There was a problem hiding this comment.
Fix: the CLI throws an exception if you don't set the cover-image flag. Are you changing the value somewhere between here and when it's used in the generator? Because if the pack.png path is set to an empty string like you've done here, it should work without throwing an exception.
|
Thank you for your review and reply! I am working on it. |
It'll be more convenient for users if this program can support generating dp/rp from the command line. So I add the feature. Users can add musics and change settings from the cli, it can be used without gui, this may make the server users happy.