Core: Allow TOML documents for player files - #6362
Draft
duckboycool wants to merge 2 commits into
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this fixing or adding?
Probably a longshot on this being desired, but I feel like there are some pretty big problems with YAML that may make this easier in the long run.
As player files are already meant to at least mostly work with JSON, this can largely just be dropped in. The biggest concern probably is that TOML does not have a
nullvalue or something that otherwise maps well to pythonNone, even though some things do use that. I've done a bandaid fix for trigger categories here to use the string"null", but this may not work everywhere. Even though I think it'd make sense to use it there as well, this would make translating thehost.yamltrickier in addition to the work needed on supporting it onGroupand such already.Since there's now conflicting formats, I think this now necessitates extension checking for local gen. I also figured it makes sense to handle json specifically so people will know if their file that was meant to be a json is only actually valid as a yaml.
If we do actually want to do this, I think rollout should probably follow something like the following:
Also, since we don't write anything for now, we could probably get away with the standard
tomllib. I think we could want to in the future though, and the functionality here to keep the state of the document intact when reading could be beneficial for things like the host config. I also think it'd be good to introduce this with TOML 1.1 support, which would have to wait until python 3.15 for stdlib. (Plus the dependency is pure-python and quite light.)How was this tested?
Tested local and webhost gen with the following player
yamltoml. Also tried to make sure some error cases were still handled sensibly.If this makes graphical changes, please attach screenshots.
👨🇱