Velocity icon indicating copy to clipboard operation
Velocity copied to clipboard

Added MiniMessage support to built-in motd

Open 4drian3d opened this issue 2 years ago • 15 comments

  • MiniMessage support has been added to build-in motd replacing legacy color codes usage
  • Bump config-version to 2.1

4drian3d avatar Mar 15 '22 01:03 4drian3d

I'm happy with this but would like some input from others before I merge.

astei avatar Mar 15 '22 03:03 astei

A prefix is really weird - is there any reason not to migrate the config?

kezz avatar Mar 15 '22 08:03 kezz

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.

kyngs avatar Mar 15 '22 08:03 kyngs

A prefix is really weird - is there any reason not to migrate the config?

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

4drian3d avatar Mar 15 '22 09:03 4drian3d

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.

Do you know what migration means? The point of migration is that nothing breaks.

to migrate the formatting of that option in a "patch" version like 3.1.2 would be strange

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?

kezz avatar Mar 15 '22 09:03 kezz

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.

Do you know what migration means? The point of migration is that nothing breaks.

I'm sorry, I understood it as if it were to just switch to mm. How do you want to migrate such a thing?

kyngs avatar Mar 15 '22 09:03 kyngs

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?

Astralchroma avatar Mar 15 '22 09:03 Astralchroma

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.

Do you know what migration means? The point of migration is that nothing breaks.

to migrate the formatting of that option in a "patch" version like 3.1.2 would be strange

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?

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

4drian3d avatar Mar 15 '22 09:03 4drian3d

How do you want to migrate such a thing?

Velocity has a versioned config, there's a million and one ways you could easily setup migrations.

kezz avatar Mar 15 '22 09:03 kezz

How do you want to migrate such a thing?

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

4drian3d avatar Mar 15 '22 10:03 4drian3d

MiniMessage can convert components to MiniMessage so it's not hard to convert it.

Astralchroma avatar Mar 15 '22 10:03 Astralchroma

migration1 The migration and the motd format are working correctly, it should be ready for another review

4drian3d avatar Mar 15 '22 10:03 4drian3d

I'm happy with this but am going to hold this off until we get a new minor version out.

astei avatar Mar 19 '22 22:03 astei

will this be implemented? (About when this version of Velocity will come out)

ReferTV avatar Apr 17 '22 19:04 ReferTV

I'm happy with this but am going to hold this off until we get a new minor version 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

4drian3d avatar Apr 17 '22 20:04 4drian3d