Skip to content

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

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

Merged

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