ArchipelagoMW / Archipelago

Archipelago Multi-Game Randomizer and Server
https://archipelago.gg
Other
412 stars 556 forks source link

Core: Allow world to display their valid keys in yaml templates #3574

Open agilbert1412 opened 2 weeks ago

agilbert1412 commented 2 weeks ago

What is this fixing or adding?

Currently, OptionSets and some other Option types inherit from VerifyKeys, and can therefore specify a list of valid keys. If they do, only these specific strings will be accepted in that option.

But in the yaml template, these keys are not shown to the user. It is therefore very difficult, if not impossible, for a user to figure out what exactly they can put in as values in that option, from just the template.

image

This PR introduces a new line in the template for these options, where the set of valid keys is displayed to the user.

image

Since the previous behavior was to not display this, and for some options of some games the list of valid keys could be absolutely massive, a new field has been added to VerifyKeys to choose if the valid keys should be displayed, on a case by case basis. This field is defaulting to False, which is the previous behavior, so this PR is not a breaking change and should not affect any world that does not opt-in to the feature.

image

Plus, I also used the feature in Stardew Valley, because well I wanted it, and also to show that it works.

How was this tested?

Generated some templates, and verified that Stardew Valley's Mods option, which has been changed in this PR to use the new feature, displays properly. Also generating templates for all worlds still works, and this is not a breaking change.

Also ran unit tests, although I'm not sure if any would check this behavior, at least it means I didn't break unrelated things.

ThePhar commented 2 weeks ago

Wouldn't this be no longer an issue with the webhost changes that now render the full lists/sets with their valid keys? Also, this is definitely just my personal preference, but I don't like it outputting a gigantic single line.

beauxq commented 2 weeks ago

Wouldn't this be no longer an issue with the webhost changes that now render the full lists/sets with their valid keys?

I think this is for the yamls that come from the Launcher "Generate Template Options" button. The webhost isn't involved.

agilbert1412 commented 2 weeks ago

@ThePhar

Wouldn't this be no longer an issue with the webhost changes that now render the full lists/sets with their valid keys? Also, this is definitely just my personal preference, but I don't like it outputting a gigantic single line.

Yes, as beauxq said, this is for the generated templates, not for the website.

As for the formatting, I wasn't sure what was better, because there can be a lot of valid keys (for example, a game like Starcraft2 who would use this on their exclude_items would be several hundreds), so potentially if it was made into a multiline block, it might be so tall that it makes the rest of the yaml very hard to use. But one line, if too long, can also be annoying. Overall, I'm not particularly attached to one way or the other, and I think both are an improvement over the current "display nothing" behavior.

NewSoupVi commented 2 weeks ago

I definitely also feel like this probably shouldn't suddenly turn a 200 char line length yaml into like, 2000

There should definitely be some sort of wrapping, or each key on its own line... Maybe it could wrap if >=15 keys and be on a single line each for <15? Or maybe the world could choose that itself, by making this an enum instead of a bool. Or maybe I'm overcomplicating things :D

I'm definitely on board with this though. Just think a little extra polish is needed in terms of display

agilbert1412 commented 2 days ago

I'm not knowledgeable enough, nor motivated enough to go obtain that knowledge, to do the polishing in question. I agree it would be an improvement, but I also firmly believe that this non-polished version is better than nothing and I don't see much reason to hold it back.

Someone else can polish it if they want to later.

If you believe it is better to not do this, than to do this in an unpolished way, I will simply close the PR.