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

Use YAML for Velocity config #414

Open
jacoballen1 opened this issue Jan 20, 2021 · 9 comments
Open

Use YAML for Velocity config #414

jacoballen1 opened this issue Jan 20, 2021 · 9 comments
Labels
type: feature New feature or request

Comments

@jacoballen1
Copy link
Contributor

jacoballen1 commented Jan 20, 2021

Note: This issue is VERY early, and any code snippets shown are hypothetical, subject to change, and aren't guaranteed to work without other code modification (where applicable).

As the title states, this issue proposes the use of YAML over HOCON as the transition to completely use Configurate begins. While many may prefer the use of HOCON over the use of YAML, HOCON is currently limited on how sorting is done (by hashcode (thanks zml)). I believe that it is critical we maintain the order of config options to ease confusion on server admin/owner(s) and keep the same clean appearance that we are all used to.

I have experimented with both options, links to each are below, and believe that YAML is the best option on the market at the time of writing. With that being said, there is always the possibility of advancements in HOCON that will allow us to control the order of the config. Along with this, I do plan on writing an example converter for converting the TOML to YAML if necessary.

YAML
HOCON

(comments are currently missing, but can be added as progression is made)
YAML Config
HOCON Config

@astei astei added the type: feature New feature or request label Jan 20, 2021
@astei astei added this to the Velocity 2.0.0 milestone Apr 18, 2021
@astei
Copy link
Contributor

astei commented Apr 23, 2021

Rather than a straight conversion of the existing TOML to YAML, I propose going with a setup like this as it fits in better with my goals for Velocity 2.0.0.

@jacoballen1
Copy link
Contributor Author

Looks good, I only did a straight conversion for POC. however, what does “pc” stand for in this case? I see it is referenced in the type and later on in the file

@astei
Copy link
Contributor

astei commented Apr 23, 2021

"pc" stands for Java Edition.

@Camotoy
Copy link
Contributor

Camotoy commented Apr 24, 2021

I understand that's a proof-of-concept, but for a proper implementation, Java Edition should probably be referenced as java-edition and Bedrock Edition as bedrock-edition. Bedrock has a PC implementation now, so calling the Java setting pc is more likely to confuse a few folks out there.

@jacoballen1
Copy link
Contributor Author

@Camotoy Great idea. I may submit a PR for this (given tux hasn't already started working on it) and have that in mind.

@jacoballen1
Copy link
Contributor Author

@astei I was going through converting the config and happened to notice you didn't include the 'try' list in this iteration. Is this intentional or a mistake?

@astei
Copy link
Contributor

astei commented May 9, 2021

It's a mistake, but I want to revisit the try list and forced host configuration anyway.

@astei astei changed the title [2.0.0] Use YAML for Velocity config Use YAML for Velocity config May 19, 2021
@leytilera
Copy link

Why should someone switch from a very good config format like TOML to a bad config format like YAML? It is just a bad decision. Please keep TOML, it is so much better.

@Strahilchu
Copy link

TOML offers more simplicity than YAML but I don't see it being a big deal or making any difference here in particular.

If there is a noticeable benefit from changing it then I agree it should be changed but otherwise don't fix something that isn't broken?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants