-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add more utility for lmlsb edit and dump #133
Conversation
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #133 +/- ##
==========================================
- Coverage 49.04% 48.07% -0.97%
==========================================
Files 23 23
Lines 5548 5672 +124
==========================================
+ Hits 2721 2727 +6
- Misses 2827 2945 +118 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I've added a couple comments on coding style issues.
In general I'll have to think about whether or not to merge this, as I'm not sure about adding another data/serialization format to the dump output.
XML is already supported as a dump format, and I get that JSON can be easier to work with then XML, but one of the reasons for using XML here (and in the original tool pylm is forked from) is that the XML format gives a much more accurate representation of the original LiveMaker editor format (when you make a game in LM, the project files are stored as XML).
I think for addressing #126 it would be sufficient to have a simple command-line based workflow that just takes something like -p/--param PR_LEFT=20
.
Being able to parse a file to set multiple lines/fields at once would be nice to have, but I think ideally we should be parsing XML (which would also enable parsing of actual/original LM project files)
One question I have is whether or not you added JSON here to facilitate something in translator++?
.gitignore
Outdated
|
||
|
||
deploy.bat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a change that's specific to your workspace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is.
src/livemaker/cli/lmlsb.py
Outdated
@@ -150,9 +152,59 @@ def validate(input_file): | |||
print(f" script mismatch, {len(orig_bytes)} {len(new_bytes)}") | |||
|
|||
|
|||
def _dump_json(lsb, pylm, jsonmode): | |||
jsonData = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use snake_case
for variable names so that your changes match the rest of the code base
(as noted in the contributing docs, the code base generally follows PEP8 Python standards with the exception of code that is intended to directly mirror original LiveMaker engine Pascal naming conventions)
Co-authored-by: Peter Rowlands (변기호) <[email protected]>
Co-authored-by: Peter Rowlands (변기호) <[email protected]>
@pmrowla In addressing the need for inputting values without manual intervention, several considerations emerged during the development process:
After much trial and consideration, I found that JSON format offered the most logical solution for my use case. I prefer it over the original XML format due to its efficiency and compatibility with other applications. And yes, to answer your question, I've already integrated this feature for Translator++. Thank you again for your feedback. I'll make sure to address your suggestions as I continue to refine the implementation. |
One thing to note would be that when pylivemaker is installed you can just use it as a library, so any scripts and utilties you need for translator++ purposes can be kept separate and just |
Thank you for the hint @pmrowla |
Thanks again for this PR. I'm closing this for now as it's not something I have the time to maintain in pylm, and since it seems like this can be handled within translator++. But I'm open to revisiting this in the future if needed. |
Resolves #126
lmlsb edit
lmlsb dump
lmlsb edit