Skip to content
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

update use textComponents, since we rely on paper anyway #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FireInstall
Copy link
Contributor

Minedown instead of MiniMessage

@@ -286,7 +286,7 @@ public void onDisable() {
}

public void loadConfig() {
saveDefaultConfig();
getConfig().options().copyDefaults(true); // save newly added config options to disk
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually do anything on its own? I would assume this needs the config to be saved afterwards. (And you wouldn't want that on loading as that would overwrite existing values) Also with the removed saveDefaultConfig I would assume that it doesn't initially save the config at all.

I don't think it really matters that much that the keys are in the config though. If they are missing they will be pulled in from the file in the jar and if someone with a config which is missing them wants to edit it they can just get the ones they want from the default file in the jar or on github.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well yes, it was missing the call to saveConfig(). That was the point where I lacked enough concentration to go to sleep.

However, in my opinion it is pretty inconvenient and confusing, for the config file to miss newly added stuff, when it is really no effort to save defaults.

Copy link
Member

@Phoenix616 Phoenix616 May 8, 2024

Choose a reason for hiding this comment

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

This will now cause your local file changes to be overwritten when running /syncinv reload as that calls loadConfig too. The only way to avoid that while still writing the new values into the config would be to first save the default config, then reload the config and then save the defaults into it again. (Or do the defaults saving in startup outside the loadConfig)

Copy link
Contributor Author

@FireInstall FireInstall May 8, 2024

Choose a reason for hiding this comment

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

We could just use the config in jar, if we don't overwrite the defaults in getLang() (would be also a problem if we would not save new config keys to disk).
In this case we just have to reload and save.

@FireInstall FireInstall force-pushed the upstreamComponents branch 3 times, most recently from e3d7337 to 51fbb14 Compare May 8, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants