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.

Sign up for GitHub

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

Jump to bottom

Options: fix yaml export corner case "6_2" #3529

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Options: fix yaml export corner case "6_2" #3529

merged 1 commit into from
Jun 14, 2024

Conversation

Berserker66
Copy link
Member

What is this fixing or adding?

https://discord.com/channels/731205301247803413/731214280439103580/1250848259677618196
Some strings like "6_2" may be dumped raw, which then get interpreted as for example 62 back. This runs it all through the yaml dumper, so if any special handling is needed it should get applied.

How was this tested?

https://discord.com/channels/731205301247803413/731214280439103580/1250857822153867264
I have since moved the instructions from the template to the new function in Options.py, but it does the same thing,

As far as I know, nothing should change for currently supported games.

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jun 13, 2024
@Exempt-Medic Exempt-Medic added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label Jun 14, 2024
TRPG0
TRPG0 approved these changes Jun 14, 2024
View reviewed changes
Copy link
Collaborator

@TRPG0 TRPG0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing because I'm pretty sure it's my fault that this PR even exists at all.

Tested both exporting from webhost and generating template options, both cases now work fine with ULTRAKILL's Goal option.

ScipioWright
ScipioWright approved these changes Jun 14, 2024
View reviewed changes
Copy link
Collaborator

@ScipioWright ScipioWright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with Ultrakill specifically, it functioned without modifying options choices that didn't start with a number.

Something that someone else should maybe test is when generating a yaml off of the webhost, that it still properly generates those options.

@ScipioWright ScipioWright added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jun 14, 2024
@Berserker66 Berserker66 merged commit 3972b1b into main Jun 14, 2024
28 checks passed
@Berserker66 Berserker66 deleted the options_corner_case_export branch June 14, 2024 22:48
sflavelle pushed a commit to sflavelle/Archipelago-tgc that referenced this pull request Jun 20, 2024
@Berserker66 @sflavelle
Options: fix yaml export corner case (ArchipelagoMW#3529)
093e6e2
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
@Berserker66 @qwint
Options: fix yaml export corner case (ArchipelagoMW#3529)
982851a
GameWyrm pushed a commit to GameWyrm/Archipelago-GW that referenced this pull request Jul 4, 2024
@Berserker66 @GameWyrm
Options: fix yaml export corner case (ArchipelagoMW#3529)
0cda6d2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ScipioWright ScipioWright ScipioWright approved these changes

@TRPG0 TRPG0 TRPG0 approved these changes

Assignees
No one assigned
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Berserker66 @ScipioWright @TRPG0 @Exempt-Medic