refactor: moving time methods to ModLoaderTime
#346
Conversation
KANAjetzt
added
enhancement
addons/mod_loader/api/time.gd
Outdated
| extends Node | ||
|
|
||
| class_name ModLoaderTime |
There was a problem hiding this comment.
| extends Node | |
| class_name ModLoaderTime | |
| class_name ModLoaderTime | |
| extends Node |
There was a problem hiding this comment.
Also please add a short comment above the class_name that describes this class.
See the top comment of log.gd for an example.
KANAjetzt
changed the title
ModLoaderTime
Co-authored-by: KANAjetzt <[email protected]>
Co-authored-by: KANAjetzt <[email protected]>
Co-authored-by: KANAjetzt <[email protected]>
| @@ -505,7 +490,7 @@ static func _rotate_log_file() -> void: | |||
| var error := log_file.open(MOD_LOG_PATH, File.WRITE) | |||
| if not error == OK: | |||
| assert(false, "Could not open log file, error code: %s" % error) | |||
| log_file.store_string('%s Created log' % _get_date_string()) | |||
| log_file.store_string('%s Created log' % ModLoaderTime.get_date_string()) | |||
There was a problem hiding this comment.
Please search the entire codebase for calls to the Time utility functions and update them to ModLoaderTime. In Godot, you can press Ctrl + Alt + F to search the entire project. Additionally, please perform a test run. You can create an empty Godot project and symlink the repository into the addons folder for testing.
There was a problem hiding this comment.
@KANAjetzt is there need for Internal Date Time functions in addons/mod_loader/setup/setup_log.gd ?
There was a problem hiding this comment.
You can keep everything related to the setup (everything in the setup directory) as it is. The setup has to be completely separate, which is why there is a lot of duplicated code in these files.
There was a problem hiding this comment.
There is one more call to _get_date_time_string()
godot-mod-loader/addons/mod_loader/api/log.gd
Line 479 in a73641f
| # This class provides utility functions to retrieve the current time, date, and date-time in specific string formats. | ||
| class_name ModLoaderTime | ||
| extends Node | ||
|
|
There was a problem hiding this comment.
One empty line missing.
If you are interested you can read up on the GDScript style guide here.
There was a problem hiding this comment.
The empty line is still missing.
Co-authored-by: KANAjetzt <[email protected]>
| # This class provides utility functions to retrieve the current time, date, and date-time in specific string formats. | ||
| class_name ModLoaderTime | ||
| extends Node | ||
|
|
There was a problem hiding this comment.
The empty line is still missing.
| @@ -505,7 +490,7 @@ static func _rotate_log_file() -> void: | |||
| var error := log_file.open(MOD_LOG_PATH, File.WRITE) | |||
| if not error == OK: | |||
| assert(false, "Could not open log file, error code: %s" % error) | |||
| log_file.store_string('%s Created log' % _get_date_string()) | |||
| log_file.store_string('%s Created log' % ModLoaderTime.get_date_string()) | |||
There was a problem hiding this comment.
There is one more call to _get_date_time_string()
godot-mod-loader/addons/mod_loader/api/log.gd
Line 479 in a73641f
|
@AlajeBash, you requested a review but did not apply my requested changes. Additionally, there is now a second PR for this? Please check my last comments and make the requested changes. Let me know if you need any help or if something is unclear. |
DESCRIPTION
I Created a new
GDScriptfile with the nametime.gdto hold theModLoaderTimeclass.And in
time.gd, i define theModLoaderTimeclass and move the time-related utility methods into it.closes #246