Add configuration option for MiniMessage-formatted MOTD#8242
Add configuration option for MiniMessage-formatted MOTD#8242wordandahalf wants to merge 2 commits into
Conversation
|
Welcome to the world of PR :3 |
|
|
||
| public Component noPermission = Component.text("I'm sorry, but you do not have permission to perform this command. Please contact the server administrators if you believe that this is in error.", NamedTextColor.RED); | ||
| + @Nullable | ||
| + public Component motd = null; |
There was a problem hiding this comment.
im not sure how is handled the null in the file... and what can happen if you use a empty string in the config..
There was a problem hiding this comment.
From my testing, the field is omitted by default in the file. If added, its value overrides that of motd in server.properties so long as it is not null/empty:
# Is removed, considered null/empty
motd:
# Same story
motd: null
# Considered a value, yields an empty MOTD
motd: ''Would it be preferred to make the field be an empty string by default, and have it override only when it is nonempty?
There was a problem hiding this comment.
I generally eer towards using a string like "default", as then we can just document the behavior of what's going on, vs empty string which, what if you do want an empty string? In the future there is an argument to move all of the config strings over into paper.yml, and I'm not 100% sure that in the long term we'll want to keep those options existing in their standard config files (outside of reading them if they exist so that we can convert them). "default" just offers a sane option which allows us to determine and change what the default behavior is in the future without worrying as much about the side-effects of it (default right now means grab from server.properties, in the future, it could be, use the default hard-coded string)
There was a problem hiding this comment.
I see your perspective, but making it nullable seems to be the simplest and most straightfoward approach. It's completely opt-in, otherwise it functions exactly as it did before. And it's pretty much self-documenting. You want a MiniMessage-formatted MOTD? Add the field. You want to use the value in server.propertes? Remove the field.
Of course, I'd be more than happy to reimplement it in the way you describe if that's what's decided is the best course of action.
Thanks for that, I managed to miss that tidbit. |
|
I've (hopefully) fixed up the configuration commit. I've noticed that my patch numbers do not correspond with the existing numbering. Is that an issue? |
|
This can be fixed by applying patches and rebuilding. In general, I see why the whole "null" is a little bit iffy. What if we had a new type that represented an "override"? Basically, This might be a useful type in general if we want to move more things to use adventure like this. |
This closes #8230 by adding a new configuration option at
messages.motdinpaper-global.ymlthat overrides the value ofmotdinserver.propertiesif it exists.This is my first contribution, so please let me know if there is anything that I can change; I feel like there may have been some curfuffling in the process of rebuilding of the patches, but it functioned as expected when I tested.
Thanks!