-
Notifications
You must be signed in to change notification settings - Fork 37
serialize into ModData and ModDetails #60
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
Conversation
What's the difference between mod_details.gd and mod_data.gd? It looks like there's code for validation in both, would all the mod-specific stuff be better in a single file instead of split across two? It might also be helpful to include a comment at the top of those files stating what their purpose is. Generally I think all funcs can benefit from comments, esp. if you're not familiar with the codebase. For static funcs, would they be better being visually separated from the non-static funcs? Ie. you could move them down the file into their own section with comment headings. Eg. I used this approach throughout mod_loader.gd (eg here):
|
oh totally forgot to sort them again. will also add comments |
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 is amazing work, well done Ste! 🏆
I only have a few minor points: Potentially change a class name, and make the logging consistent
addons/mod_loader/mod_details.gd
Outdated
|
||
if re.search(version_number) == null: | ||
printerr('Invalid semantic version: "%s". ' + | ||
'You may only use numbers and periods in this format {mayor}.{minor}.{patch}' % version_number) |
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.
Is it possible to show the mod name? That would make debugging much easier, esp. for logs like this which atm don't tell you which mod they're for
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.
good point, will do
will wait with merging until we have a logger class with static methods to log from, as those seem to work in engine and exported and with automatic setup. |
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.
Looks good to me. Like you said, there's a few bits left to do, but they can be raised in a subsequent PR
@Qubus0 I don't think we should use godot 4 documentation comments yet. The VS Code plugin for Godot already shows comments, but it doesn't handle the Eg this uses the normal But it becomes this if you use So I think we should hold off on using them until the plugin is fixed |
hm that's a bummer. I guess it also doesn't handle the bbcode references to classes and methods? Looks like it's treating two ## as markdown heading |
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.
I think everything I mentioned before has been sorted here
And again, really great work Ste :)
Does this account for leading zeroes? e.g. |
@MythicManiac not yet, but I can add it. Probably should also disallow numbers over 999, since right now versioning a la LaTeX is still possible (0.3.1415, adding a digit of pi for every update) |
for better type safety and convenience
this is only internal, nothing needs to be changed in mods.
@ithinkandicode check if this works for you to also integrate the config stuff
also closes #54