-
-
Notifications
You must be signed in to change notification settings - Fork 619
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
Added MiniMessage support to built-in motd #666
Conversation
4drian3d
commented
Mar 15, 2022
•
edited
Loading
edited
- MiniMessage support has been added to build-in motd replacing legacy color codes and json usage
- Bump config-version to 2.6
proxy/src/main/java/com/velocitypowered/proxy/config/VelocityConfiguration.java
Outdated
Show resolved
Hide resolved
I'm happy with this but would like some input from others before I merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a contributor, but this seems like a great change.
A prefix is really weird - is there any reason not to migrate the config? |
That would mean peoples motd's breaking on a minor update. Not having the prefix would be a good idea on velocity 4.x IMO. |
I thought it would be the best option, to migrate the formatting of that option in a "patch" version like 3.1.2 would be strange |
Do you know what migration means? The point of migration is that nothing breaks.
Then let's bump the minor version? I assume Velocity, being a modern proxy, would want to migrate away from legacy formatting eventually, so why not do it in one step? |
I'm sorry, I understood it as if it were to just switch to mm. How do you want to migrate such a thing? |
I made a similar pull request to Paper, and after discussion there the course of action was to forcibly migrate the config messages instead of maintain compatibility with both, would it not be best to be consistent and forcibly migrate the format with Velocity? |
Yes, the ideal would be to migrate at once from all legacy color code uses to MiniMessage, but being a minor version to be released in proximity, I thought better to use a prefix, but if required, I could try to migrate to MiniMessage in that option now |
Velocity has a versioned config, there's a million and one ways you could easily setup migrations. |
Honestly, I never paid attention to this configuration option, I just remembered it existed, now I am making the migration |
MiniMessage can convert components to MiniMessage so it's not hard to convert it. |
I'm happy with this but am going to hold this off until we get a new minor version out. |
will this be implemented? (About when this version of Velocity will come out) |
Considering that pull request #692 was accepted and that very soon pull request #690 will be accepted, I believe that the next version will no longer be considered as a "patch" and this pull request could be accepted directly. Also, it would be nice if the first version that includes MiniMessage already has its use implemented in all the places where the legacy format was used (where possible) which is the only place that would be missing |
a8c024d
to
7b113ad
Compare