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

WebHost: Fix Named Range displays on Player Options page #3521

Merged
merged 6 commits into from
Jun 13, 2024

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Jun 12, 2024

As with my other two WebHost PRs, I have no idea if this is "structurally" the best fix.

Note: Just telling worlds to change the option names in special_range_names to what they want does not work. If any of the names in there contain capital letters, your yaml will never generate (unless you use the numeric values instead)

Before:

image

After:

image

@github-actions github-actions bot added affects: webhost Issues/PRs that touch webhost and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jun 12, 2024
@NewSoupVi NewSoupVi added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. affects: release/blocker Issues/PRs that must be addressed before next official release. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jun 12, 2024
@NewSoupVi NewSoupVi requested a review from LegendaryLinux June 12, 2024 22:15
@NewSoupVi NewSoupVi added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Jun 12, 2024
Copy link
Contributor

@Jouramie Jouramie left a comment

Choose a reason for hiding this comment

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

Tho I do not disagree with the title part, I think the underscores in the special ranges are just a Stardew thing. Specifically with the Chefsanity option, since it was converted to a special range in 5.x.x, and the option names copied and pasted in the special ranges.

Also, this change should probably be applied as well to the weighted options

image

@NewSoupVi
Copy link
Member Author

Oh, you're saying this is actually intentional on Stardew's part? I thought this was a bug

If that's the case, I will just close this

@NewSoupVi
Copy link
Member Author

Confirmed that this is not a bug and Stardew just chose to have its names like this

@NewSoupVi NewSoupVi closed this Jun 13, 2024
@Exempt-Medic
Copy link
Member

Exempt-Medic commented Jun 13, 2024

I asked Kaito beforehand about this, and it's worth noting that Stardew uses inconsistent cormatting then (see StartingMoney), all of which are displayed in a uniform fashion on the regular site. Regardless of anything specific to Stardew though, the new WebHost changes this for every game, Lufia 2 being a good example but also progression_balancing for every single game. I think this PR should remain open because it restores the 0.4.6 behavior for displaying special values in NamedRanges

@NewSoupVi
Copy link
Member Author

Reopening because my suggested world-side fix doesn't work

If you have capital letters in special_range_names, your yamls using those option names will never generate.

@NewSoupVi NewSoupVi reopened this Jun 13, 2024
@github-actions github-actions bot added the affects: core Issues/PRs that touch core and may need additional validation. label Jun 13, 2024
@LegendaryLinux LegendaryLinux self-assigned this Jun 13, 2024
@NewSoupVi
Copy link
Member Author

NewSoupVi commented Jun 13, 2024

In fact, I just realised the docstring of the NamedRange class says that you can't use upper case letters

So if that is going to be the case, we do at least need the call to title, because worlds can't do that part themselves.

Also yes, I should apply this to weighted options as well

@NewSoupVi
Copy link
Member Author

Ok, I think I got everything the way I want it now.

@LegendaryLinux LegendaryLinux merged commit 533395d into main Jun 13, 2024
25 checks passed
sflavelle pushed a commit to sflavelle/Archipelago-tgc that referenced this pull request Jun 20, 2024
…MW#3521)

* Player Options: Fix Named Range displays

* Also add validation to the NamedRange class itself

* Don't break Stardew

* Comment

* Do replace first so title works correctly

* Bring change to Weighted Options as well
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
…MW#3521)

* Player Options: Fix Named Range displays

* Also add validation to the NamedRange class itself

* Don't break Stardew

* Comment

* Do replace first so title works correctly

* Bring change to Weighted Options as well
GameWyrm pushed a commit to GameWyrm/Archipelago-GW that referenced this pull request Jul 4, 2024
…MW#3521)

* Player Options: Fix Named Range displays

* Also add validation to the NamedRange class itself

* Don't break Stardew

* Comment

* Do replace first so title works correctly

* Bring change to Weighted Options as well
AustinSumigray pushed a commit to AustinSumigray/Archipelago that referenced this pull request Jan 4, 2025
…MW#3521)

* Player Options: Fix Named Range displays

* Also add validation to the NamedRange class itself

* Don't break Stardew

* Comment

* Do replace first so title works correctly

* Bring change to Weighted Options as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. affects: release/blocker Issues/PRs that must be addressed before next official release. affects: webhost Issues/PRs that touch webhost and may need additional validation. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants