Skip to content

Conversation

@ithinkandicode
Copy link
Collaborator

@ithinkandicode ithinkandicode commented Mar 2, 2023

Moves all config JSON methods to a dedicated class, following the discussion in #153.

Like #156, this PR also uses a new directory called api.

I have tested this refactor and can confirm that it is working as before.

Other Changes

  1. Adds a deprecation message to the old method.
    This is currently commented, pending feat: ✨ API: Add methods for deprecation + API Folder #156

  2. Fixes an error: save_dictionary_to_file should have been save_dictionary_to_json_file.
    Aside from this, there are no other changes from the version currently on the development branch.

Breaking Changes

Deprecates ModLoader.get_mod_config. However, Brotato is the only game that contains this method, and its implementation ATOW is broken, such that loading a custom config JSON file does not work, so there aren't any mods that use it.

Also fixes an error:
`save_dictionary_to_file` should have been `save_dictionary_to_json_file`.
@ithinkandicode ithinkandicode added the refactor / cleanup Improves readability or maintainability label Mar 2, 2023
@ithinkandicode ithinkandicode changed the title API: Move Config to a dedicated class ❌ API: Move Config to a dedicated class Mar 2, 2023
@ithinkandicode
Copy link
Collaborator Author

ithinkandicode commented Mar 2, 2023

I have added a temporary ❌ to the PR name to indicate that another PR should be merged first: #156.

Once #156 is merged, the deprecation message in mod_loader.gd (here) can be uncommented.

@ithinkandicode
Copy link
Collaborator Author

ithinkandicode commented Mar 2, 2023

There's an issue here as I've used ModLoader.logged_messages. I would have liked to have put it in config.gd, but I can't affect its member variables from these static funcs. However, it won't work at all if someone tries to use get_mod_config in their mod's _init, because it'll be before ModLoader exists. (Edit: Same with ModLoader.os_configs_path_override)

What would you guys suggest?

EDIT: @KANAjetzt Suggested the method outlined in #161, which is another autoload singleton to store data. This is a perfect fix IMO.

Copy link
Collaborator

@Qubus0 Qubus0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good.
We might want to consider refactoring it work as a resource like ModManifest, but that is something for later

@ithinkandicode ithinkandicode changed the title ❌ API: Move Config to a dedicated class API: Move Config to a dedicated class Mar 2, 2023
@ithinkandicode ithinkandicode requested a review from Qubus0 March 2, 2023 16:57
Copy link
Collaborator

@Qubus0 Qubus0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little thing, rest is good

@ithinkandicode ithinkandicode enabled auto-merge March 2, 2023 17:25
@ithinkandicode ithinkandicode added this pull request to the merge queue Mar 2, 2023
Merged via the queue into GodotModding:development with commit dd92e42 Mar 2, 2023
@ithinkandicode ithinkandicode deleted the api-config-json branch March 2, 2023 17:26
@ithinkandicode ithinkandicode changed the title API: Move Config to a dedicated class refactor: ♻️ API: Move Config to a dedicated class Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor / cleanup Improves readability or maintainability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants