-
-
Notifications
You must be signed in to change notification settings - Fork 348
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
Change config data from YAML to JSON #5444
Conversation
Hmm, so you decided to not give net.mamoe.yamlkt a chance but default to making all config files JSON. In the linked topic, there was one issue with JSON and that was the inability to have comments in it. Are all these files free of comments, so we actually do not need comments? |
It looks like: Yes, free of comments, except all these |
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.
Code looks good so far, although IMO it does not make sense to convert part of the config files to JSON but not all. Exchanging YAML with JSON should be done in one PR.
If it is done at all. I have no particularly strong opinion about it, but trying first if net.mamoe.yamlkt
works fine as multiplatform replacement for kaml seems to me results in less work (TBH I couldn't think of a reason why it shouldn't work). But, as I wrote, if you prefer to get rid of config files in YAML altogether, that's fine, too.
|
||
inline fun <reified T> Resources.getYamlObject(@RawRes id: Int): T = | ||
Yaml.default.decodeFromStream(openRawResource(id)) | ||
|
||
/** shortcut for [getYamlObject] with included type information */ | ||
fun Resources.getYamlStringMap(@RawRes id: Int): Map<String, String> = this.getYamlObject(id) | ||
|
||
@OptIn(ExperimentalSerializationApi::class) |
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.
what's experimental here?
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.
The Json.decodeFromStream(stream)
function that infers the deserializer from the output type instead of an explicit parameter.
I actually prefer JSON over YAML, because YAML – while looking easy and simple – is actually a complex and sometimes counter-intuitive data format, see the yaml document from hell. Also, getting rid of the YAML parser would decrease the app size a little, which would be nice. That's why I didn't look into Now I did and it looks like |
Yeah, YAML kind of gets constant flak. |
Well, if we write our own files then possibility of creating insane constructs is lesser problem. If we would be getting external data then this would be a good reason to expect them in JSON. And JSON has big flaws of banning trailing comma and missing comment support. Where JSON is much worse when you try to document something (comment needs to be stuffed into nonfunctional fields, ugly diffs). |
What about jsonc or json5? |
Things that need comments could be defined in YAML or TOML in e.g. /res/ or in a different repo, such as it is done for the countrymetadata (do we need languagemetadata?) and then translated to JSON in a build job. I think there are not that many things that actually need comments. See #5444 (comment) |
29b530b
to
02c9f4c
Compare
kaml now supports multiplatform native (starting with the upcoming release) |
Part of