feat: refactor numerous areas and tokenize string literals where possible; also fixes some small bugs #487
Conversation
…ible; also fixes some small bugs
| @@ -4,6 +4,17 @@ extends Node | |||
| class_name ESCBaseCommand | |||
|
|
|||
|
|
|||
| # Regex for creating command name based on the script's filename | |||
| const COMMAND_NAME_REGEX = "(.+)\/([^.]+)(\\.[^.]*$|$)" | |||
There was a problem hiding this comment.
e.g. (?<path>.+)\/(?<file>[^.]+)(?<extension>\\.[^.]*$|$)
There was a problem hiding this comment.
Indeed it is, and my push from yesterday addresses it. I think that's why GH marks it as "outdated".
| var files = "- %s" % log_file.get_path_absolute() | ||
| var message = ProjectSettings.get_setting( | ||
| "escoria/debug/crash_message" | ||
| var files = "- %s" % log_file.get_path_absolute() |
There was a problem hiding this comment.
Looks as if there's some whitespace at the end of this line.
addons/escoria-core/game/escoria.gd
Outdated
| dialog_managers | ||
| ) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Remove the whitespace in this line.
| # UI-related Escoria project settings | ||
| const _UI_ROOT = "ui" | ||
|
|
||
| const DEFAULT_DIALOG_TYPE = "%s/%s/default_dialog_type" % [_ESCORIA_SETTINGS_ROOT, _UI_ROOT] |
There was a problem hiding this comment.
Should we add the settings descriptions here as well? That would make it easy to document it, because this file would be API-documented anyway.
(Doesn't need to be in this PR, we can add them in a separate issue)
There was a problem hiding this comment.
Makes sense to me. I agree re adding a new issue.
| var inventory_file = "%s/%s.tscn" % [ | ||
| ProjectSettings.get_setting( | ||
| "escoria/ui/items_autoregister_path" | ||
| var inventory_file = "%s/%s.tscn" % [ |
There was a problem hiding this comment.
Remove the whitespace on this line
| if name.empty(): | ||
| name = ProjectSettings.get_setting( | ||
| "escoria/ui/default_transition" | ||
| if name.empty(): |
There was a problem hiding this comment.
Remove the whitespace on this line
| @@ -37,85 +37,86 @@ func _ready(): | |||
|
|
|||
| # Prepare the settings in the Escoria UI category | |||
| func set_escoria_ui_settings(): | |||
| escoria.register_setting( | |||
| "escoria/ui/default_dialog_type", | |||
| escoria.project_settings_manager.register_setting( | |||
There was a problem hiding this comment.
I wonder if we should move all this into ESCProjectManager and just call escoria.project_settings_manager.initialize_settings()
There was a problem hiding this comment.
Up for discussion on that. :) I guess that depends on how fine-grained we want to get with breaking up settings (I think).
|
It's confusing for me to comment in the middle of the development. I'd say, you're on the right track here, so I'll just review when you're done. |
| [ | ||
| _transition.get_command_name(), | ||
| escoria.project_settings_manager.get_setting( | ||
| escoria.project_settings_manager.DEFAULT_TRANISITION) |
There was a problem hiding this comment.
Put the closing bracket on line 290 and align it with line 288.
| [ | ||
| _transition.get_command_name(), | ||
| escoria.project_settings_manager.get_setting( | ||
| escoria.project_settings_manager.DEFAULT_TRANISITION) |
| @@ -4,6 +4,17 @@ extends Node | |||
| class_name ESCBaseCommand | |||
|
|
|||
|
|
|||
| # Regex for creating command name based on the script's filename | |||
| const COMMAND_NAME_REGEX = "(.+)\/([^.]+)(\\.[^.]*$|$)" | |||
|
Incredible work!! Thanks a lot!! ! |
…ible; also fixes some small bugs (godot-escoria#487) Co-authored-by: Duncan Brown <[email protected]>
something missed from PR #487
something missed from PR #487
something missed from PR #487
something missed from PR #487
something missed from PR #487
No description provided.