Splitting up long files (utils/core) #153

ithinkandicode · 2023-02-28T20:38:31Z

Done	Dir	File	Class Name	Notes
✅	`api`	log.gd	ModLoaderLog	done in #205
✅	`api`	mod.gd	ModLoaderMod	All helper functions used by mod devs - #208 `ModLoader` -> `ModLoaderMod`
✅	`api`	config.gd	ModLoaderConfig	done in #157
✅	`api`	deprecated.gd	ModLoaderDeprecated	done in #156
✅	`internal`	cli.gd	_ModLoaderCLI	#226
✅	`internal`	path.gd	_ModLoaderPath	#226
✅	`internal`	file.gd	_ModLoaderFile	Including the saving funcs from #143. Maybe JSON funcs too? - done in #226
✅	`internal`	dependencies.gd	_ModLoaderDependencies	done in #222
✅	`internal`	script_extension.gd	_ModLoaderScriptExtension	done in #225
✅	`internal`	godot.gd	_ModLoaderGodot	Eg. `check_autoload_position()` - done in #172
✅	`internal/third_party`	steam.gd	_ModLoaderSteam	See related chat in #148, here - done in #158

Breaking Changes & Deprecation

Ofc, if we moved the logger funcs then every currently published Brotato mod would break. But we could handle this with a deprecation notice, which can be handled with a new func and that triggers a fatal error in the editor. I've explained how this could work in more detail in #154:

Discussion: Deprecation #154

The text was updated successfully, but these errors were encountered:

ithinkandicode · 2023-02-28T23:57:34Z

Additionally, perhaps it would be a good idea to also split up mod_loader.gd? Especially the public API methods.

And just as the utility funcs could have a directory called utils, these could directories called internal and api, for internal methods and public methods respectively.

Here's a table with my notes on class names and grouping (all applies to mod_loader.gd except ModLoaderLog).

Class	Dir	Notes
`ModLoaderInternal`	`internal`	Internal methods (ie. ones that are currently prefixed with `_*`)
`ModLoaderData`	`internal`	Data store for the local vars from mod_loader.gd (eg. `os_mods_path_override`).
`ModLoaderOptions`	`internal`	ML Options (PR #145)
`ModLoaderMod`	`api`	Public methods related to installing/setting up mods (eg. `ModLoaderMod.install_script_extension`)
`ModLoaderConfig`	`api`	Config JSON
`ModLoaderLog`	`api`	Probably deserves its own class, separate from other utils, as it's used so often

Using ModLoaderData would fix an issue where currently, if you run a func from mod_loader_utils.gd while in ModLoader's _init(), there's no way to access local vars from mod_loader.gd -- or maybe there is, but it's not obvious how.

As mentioned before, this would be completely breaking for a lot of mods, but we can take the deprecation approach mentioned in the original post above ("Breaking Changes").

Qubus0 · 2023-03-01T08:58:38Z

I largely agree. Those are some good splits. A few things:
We should keep away from misc and just keep one generic ModLoaderUtils.
Third party should not be lumped together, but we can put it in a separate directory /third_party_integration. ModLoaderUtilsSteamWorkshop, ModLoaderUtilsThunderstore. These names are getting very long though.
Splitting those also allows us to use the Steam singleton from GodotSteam, since it won't break if it is not available if the file is never parsed.

Splitting the ModLoader is another thing. If we use ModLoaderMod (which I would love) we might also have to use a second autoload. One more step in the setup process.

ithinkandicode · 2023-03-01T16:52:19Z

For the utils classes, would it be better to get rid of the Utils prefix? That would make them a bit shorter, so faster to type. Eg:

ModLoaderCLI
ModLoaderFile
ModLoaderSteamWorkshop

Perhaps then we could have the utility funcs in the api directory, as they're technically all public API methods. Could save us needing to discuss/decide what constitutes an API vs. utility method. The downside would be that the api directory might feel overly cluttered with utility funcs

@Qubus0 How come ModLoaderMod would need to be another autoload?

Qubus0 · 2023-03-01T18:01:05Z

Shorter seems fine to me.
If we want to provide access to ModLoaderMod like ModLoader like we do now, it has to be an autoload.
I did just check though, and we can just make every helper method static by just using ModLoader.*.. For example using ModLoader._saved_objects.append(packed_scene) in save_scene makes it "static"
So, a global class also works

ithinkandicode · 2023-03-02T03:54:57Z

I think that we should publish a release before we start work on a large refactor like this. We're going to lose a lot of commit history by moving things around, so it would be really good to have a point in time (ie., a release tag) that we can refer back to if we want to see the pre-refactor history.

Edit: Actually, we have the recent 5.0.1, so it's probably fine

KANAjetzt · 2023-03-02T08:15:58Z

Sorry, I'm late to the party.
What about using ModLoader and ModLoaderUtils as index files?
And just link from there to the actual Class methods?
No breaking changes and keeps the main files somewhat clean.

KANAjetzt · 2023-04-04T10:16:43Z

Rename save_scene()

Qubus0 yesterday while we are at moving this here anyway, why don't we rename it as well? save_scene is way too generic. at least save_modified_scene, but that is still a bit unclear... take_over_scene?

Move other local vars to ModLoaderStore

ithinkandicode 17 hours ago [...] Maybe it would be good to move the other local vars from ModLoader to ModLoaderStore, [...]

Based on the discussion in GodotModding#153 - Some compromises had to be done to prevent cyclic reference errors. No `ModLoaderUtils` functions could be used.

* refactor: ♻️ created LogManager and LogEntry Class Moved most of the logging logic from `ModLoaderUtils` into `LogManager`. New `LogEntry` Class to store log message info. Added log storing in `LogManager`. works towards #140 * refactor: ♻️ Moved log functions to `ModLoaderLog` Based on the discussion in #153 - Some compromises had to be done to prevent cyclic reference errors. No `ModLoaderUtils` functions could be used. * refactor: ♻️ move `MOD_LOG_PATH` into *log.gd* * refactor: ♻️ copied `ModLoaderUtils` into *mod_loader_setup.gd* Had to copy the required utility functions into the setup script, because `ModLoaderUtils` depends now on other Classes that don't get loaded. * refactor: ♻️ Moved `ModLoaderLogEntry` inside `ModLoaderLog` With that the ModLoaderLog Class is fully independed and can be loaded in the setup script. * fix: ✏️ fixed search and replace errors accidentally removed the `log_` from the deprecation message and `log_debug_json_print`. * refactor: ♻️ created *setup_log.gd* and setup_utils.gd* There tradeoffs for using the existing ModLoader Classes in the setup script are to big. So the required functions where moved into the *setup/* dir. * refactor: 🔥 clean up logged signal * refactor: ♻️ Add `existing_entry` var * feat: ✨ added time comparison Added `time_stamp` to `ModLoaderLogEntry` and instead of saving the `time` string in `time_stamps` the same log entries are now stored in `stack`. Added `ModLoaderLogCompare` inner class, allowing to call the custom sort function. * feat: ✨ Added `get_all_entries()` to `ModLoaderLogEntry` * feat: ✨ added `get_all_as_resource_array()` * feat: ✨ Added `get_all_as_string_array()` * refactor: 🔥 removed `time_stamps` in *setup_log.gd* not needed in the setup logs * fix: 🐛 not using `.keys()` * feat: ✨ Added `get_by_mod_as_resource_array()` * feat: ✨ Added `get_by_mod_as_string_array()` * feat: ✨ Added `get_by_type_as_resource_array()` * feat: ✨ Added `get_by_type_as_string_array()` * style: ✏️ remove `_array` from func names * refactor: ♻️ Added inner class `ModLoaderLogStoreUtils` To prevent some of the duplications * style: 🎨 direct return in `get_all_as_string()` to match the other funcs * refactor: ♻️ Use `ModLoaderLogStoreUtils` for all get funcs * refactor: ♻️ Removed inner class `ModLoaderLogStoreUtils` * fix: ✏️ fixed accidentally remove func header * refactor: ♻️ Updated *mod.gd* to use `ModLoaderLog` * chore: ⏪ moving self setup changes to separate PR * fix: 🦺 check if `.by_mod` has `mod_name` * style: ✏️ fix comments Added description for `ignored_mod_names_in_log` in `ModLoaderStore` and removed the `;` in the `_rotate_log_file()` comment. * style: 🎨 `get_entry()` using `str()` now

KANAjetzt · 2023-04-21T22:27:33Z

Should we move these functions, currently in mod_loader.gd, as follows:

_ModLoaderScriptExtension
- _reset_mods()
- _reload_mods()
_ModLoaderGodot
- _check_autoload_positions()
_ModLoaderSteam
- _load_steam_workshop_zips()
_ModLoaderFile
- _load_mod_zips()
- _load_zips_in_folder(folder_path: String)
ModLoaderConfig
- _load_mod_configs()

Qubus0 · 2023-04-22T11:42:53Z

_ModLoaderScriptExtension no. Unless we move all other script extension methods with it.. but to me it seems like ModLoader is the correct class for the job
_ModLoaderGodot no, i'd rather rename _get_autoload_array -> get_autoload_array, since the class is already internal and the methods are only used outside this class (not private)
_ModLoaderSteam yes.also keep the _ since it's internal but in a public class
_ModLoaderFile sure, but remove the _
ModLoaderConfig yes, keep the _

ithinkandicode · 2023-04-27T21:01:39Z

Closed by:

KANAjetzt · 2023-04-28T06:59:07Z

Let's keep this issue open until we are satisfied with the location of each file and until the comment from above regarding the functions in mod_loader.gd has been addressed.

KANAjetzt · 2023-06-08T09:37:16Z

closed by *see #153 (comment)

ithinkandicode added question / discussion Further information is requested refactor / cleanup Improves readability or maintainability labels Feb 28, 2023

ithinkandicode mentioned this issue Feb 28, 2023

feat: ✨ Workshop Support #148

Merged

ithinkandicode changed the title ~~Discussion: Splitting up utilities~~ Discussion: Splitting up long files (utils/core) Mar 1, 2023

ithinkandicode mentioned this issue Mar 1, 2023

Discussion: Deprecation #154

Closed

This was referenced Mar 2, 2023

feat: ✨ API: Add methods for deprecation + API Folder #156

Merged

refactor: ♻️ API: Move Config to a dedicated class #157

Merged

refactor: ♻️ API: Move Steam to a dedicated class #158

Merged

ithinkandicode mentioned this issue Mar 2, 2023

ModLoaderStore - Singleton for to store data #161

Closed

ithinkandicode self-assigned this Mar 2, 2023

KANAjetzt mentioned this issue Mar 23, 2023

v6.0.0 - Roadmap #181

Closed

37 tasks

KANAjetzt added this to the v6.0.0 milestone Mar 23, 2023

ithinkandicode mentioned this issue Apr 1, 2023

refactor: ♻️ Move main mod API funcs to ModLoaderMod, from ModLoader #208

Merged

KANAjetzt mentioned this issue Apr 2, 2023

refactor: ♻️ Logging + ✨ Store Logs #205

Merged

28 tasks

KANAjetzt added breaking Breaking change and removed question / discussion Further information is requested labels Apr 4, 2023

KANAjetzt changed the title ~~Discussion: Splitting up long files (utils/core)~~ Splitting up long files (utils/core) Apr 4, 2023

This was referenced Apr 19, 2023

refactor: ♻️ added ModLoaderScriptExtension Class #225

Merged

refactor: ♻️ Split up ModLoaderUtils #226

Merged

refactor: 🚚 Moved and renamed ModLoaderSteam #230

Merged

KANAjetzt mentioned this issue Apr 21, 2023

refactor: ♻️ move consts from ModLoader to ModLoaderStore #233

Merged

ithinkandicode closed this as completed Apr 27, 2023

KANAjetzt reopened this Apr 28, 2023

KANAjetzt mentioned this issue Jun 3, 2023

refactor: 🚚 move funcs out of ModLoader #255

Merged

KANAjetzt closed this as completed Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Splitting up long files (utils/core) #153

Splitting up long files (utils/core) #153

ithinkandicode commented Feb 28, 2023 •

edited by KANAjetzt

Loading

ithinkandicode commented Feb 28, 2023 •

edited

Loading

Qubus0 commented Mar 1, 2023

ithinkandicode commented Mar 1, 2023 •

edited

Loading

Qubus0 commented Mar 1, 2023

ithinkandicode commented Mar 2, 2023 •

edited

Loading

KANAjetzt commented Mar 2, 2023

KANAjetzt commented Apr 4, 2023

KANAjetzt commented Apr 21, 2023

Qubus0 commented Apr 22, 2023

ithinkandicode commented Apr 27, 2023 •

edited by KANAjetzt

Loading

KANAjetzt commented Apr 28, 2023

KANAjetzt commented Jun 8, 2023

Splitting up long files (utils/core) #153

Splitting up long files (utils/core) #153

Comments

ithinkandicode commented Feb 28, 2023 • edited by KANAjetzt Loading

Breaking Changes & Deprecation

ithinkandicode commented Feb 28, 2023 • edited Loading

Qubus0 commented Mar 1, 2023

ithinkandicode commented Mar 1, 2023 • edited Loading

Qubus0 commented Mar 1, 2023

ithinkandicode commented Mar 2, 2023 • edited Loading

KANAjetzt commented Mar 2, 2023

KANAjetzt commented Apr 4, 2023

KANAjetzt commented Apr 21, 2023

Qubus0 commented Apr 22, 2023

ithinkandicode commented Apr 27, 2023 • edited by KANAjetzt Loading

KANAjetzt commented Apr 28, 2023

KANAjetzt commented Jun 8, 2023

ithinkandicode commented Feb 28, 2023 •

edited by KANAjetzt

Loading

ithinkandicode commented Feb 28, 2023 •

edited

Loading

ithinkandicode commented Mar 1, 2023 •

edited

Loading

ithinkandicode commented Mar 2, 2023 •

edited

Loading

ithinkandicode commented Apr 27, 2023 •

edited by KANAjetzt

Loading