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

Pokemon Emerald: Adjust options #3278

Merged
merged 7 commits into from
Jun 1, 2024

Conversation

Zunawe
Copy link
Collaborator

@Zunawe Zunawe commented May 8, 2024

What is this fixing or adding?

In anticipation of #2614 being able to display more options on the main options page, I went through the Emerald options to add clarity and make things look a little nicer.

  • Removing the newlines adjacent to the """s I guess makes them render differently on the site, to get the automatic intents between lines. Figured out why this is; reverted the change.
  • Fixed the easter_egg option not having a display name.
  • Made things more consistent overall.
  • Clarified some options. Especially to add information that you wouldn't know without having seen the changelog.
  • Changed valid_keys to lists so that they're ordered for the user's benefit.
  • Removed some sections on OptionSets that don't need to exist on the website.
  • Added a full alias for HM and TM compatibility.
  • Downgraded HmRequirements from TextChoice to Choice, since custom entries have no effect.

How was this tested?

Generated templates, read through the options page on the site in a branch with #2614 also merged.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label May 8, 2024
@PoryGone
Copy link
Collaborator

PoryGone commented May 8, 2024

Removing the newlines adjacent to the """s I guess makes them render differently on the site, to get the automatic intents between lines.

Is this behavior not a WebHost bug?

@Zunawe
Copy link
Collaborator Author

Zunawe commented May 8, 2024

I'm unsure. I didn't try to track down why the appearance is different, I only saw that it was when comparing my own option tooltips with the one for progression balancing.

@Exempt-Medic
Copy link
Member

That definitely should be some kind of bug, since it means violating PEP8 is ideal

@Exempt-Medic
Copy link
Member

I assume you're aware of this, but just in case, removing the lists of valid keys from the option descriptions means that template yamls will not be easily usable on their own

@Exempt-Medic Exempt-Medic added is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: other Issue/PR is waiting for something else, like another PR. labels May 8, 2024
@Zunawe
Copy link
Collaborator Author

Zunawe commented May 9, 2024

Yeah. The tooltip is just cluttered and long with the list of possible values included. I'm considering inverting the option so that including the value makes the roadblock present, and then setting the default value to all true. That way the template automatically shows all options. But that's a breaking change, so I wouldn't want to do it in this PR.

I'm removing waiting-on: other because this PR isn't dependent on 2614, it's just relevant to a future where 2614 is merged. It would be better if it didn't have to wait on 2614 so that the clarifications make it in.

I also might even consider this a docs PR instead of an enhancement, but that distinction isn't really important here.

@Zunawe Zunawe removed the waiting-on: other Issue/PR is waiting for something else, like another PR. label May 9, 2024
@Exempt-Medic
Copy link
Member

Exempt-Medic commented May 9, 2024

It's definitely mostly docs. But there's also valid_keys, TextChoice, and the full aliases. I also will still say that several of these options now violate PEP 8:

Note that most importantly, the """ that ends a multiline docstring should be on a line by itself:

@Exempt-Medic
Copy link
Member

Exempt-Medic commented May 9, 2024

Since option set are now sorted on webhost, you could remove those changes

@Exempt-Medic
Copy link
Member

There is also no display difference between these, so your multiline docstrings are fine to put the closing """ on a separate line.

    """Adds Badges to the pool

    Vanilla: Gym leaders give their own badge
    Shuffle: Gym leaders give a random badge
    Completely Random: Badges can be found anywhere"""

Screenshot 2024-05-09 082803

    """Adds Badges to the pool

    Vanilla: Gym leaders give their own badge
    Shuffle: Gym leaders give a random badge
    Completely Random: Badges can be found anywhere
    """

Screenshot 2024-05-09 082842

@Exempt-Medic
Copy link
Member

Exempt-Medic commented May 9, 2024

Several of these changes are also 120-character line length violations that don't actually change the display.

    """Adds most key items to the pool. These are usually required to unlock a location or region (e.g. Devon Scope, Letter, Basement Key)"""

Screenshot 2024-05-09 084009

    """
    Adds most key items to the pool.
    These are usually required to unlock a location or region (e.g. Devon Scope, Letter, Basement Key)
    """

Screenshot 2024-05-09 084125

    """Adds berry trees to the pool. Empty soil patches are converted to locations and contribute Sitrus Berries to the pool."""

Screenshot 2024-05-09 084412

    """
    Adds berry trees to the pool. Empty soil patches are converted to locations and contribute Sitrus Berries to the pool.
    """

Screenshot 2024-05-09 084515

@Zunawe
Copy link
Collaborator Author

Zunawe commented May 17, 2024

Since option set are now sorted on webhost, you could remove those changes

While technically true (now that it would also retain order if keys are ordered), I see no real harm in keeping them explicitly sorted. If I add other "shortcuts" to the blacklists, I may want to order them in a manner other than alphabetical.


There is also no display difference between these, so your multiline docstrings are fine to put the closing """ on a separate line.

I did a little more investigating and found out why there's a difference if you have a new line between the opening """ and the first sentence. (I think this is only relevant behavior for 2614.) It's the use and behavior of textwrap.dedent, which removes whitespace after a newline according to how much whitespace there is on the first line. So if your docstring starts with \n , it strips 4 spaces from the start of every line. And if your docstring starts with an alphabetical character, it does not strip any whitespace. Which is why the progression balancing docstring looks indented, because it still is.

So one could add extra indentation in their own descriptions if they wanted that behavior. I think I'm going to settle on using - as bullet points to distinguish lines with option values and say that the docstrings for generic options should probably eventually get updated.


Several of these changes are also 120-character line length violations that don't actually change the display.

As long as linebreaks in the docstring correspond with linebreaks on the web page, I'm probably going to mostly ignore the 120-character limit for specifically these docstrings. Maybe I'll set a hard limit of like 180, since most long sentences or groups of long sentences can probably fit in there or be split into new sentences or paragraphs. I also don't want to add custom "matching" line breaks because it doesn't work if the CSS changes out from under me. The better option would be for the webhost code to support merging multi-line sentences somehow.

@Zunawe Zunawe force-pushed the emerald-option-descs branch from 9ed04e3 to bb3fdc1 Compare May 17, 2024 19:12
Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

Looks awesome to me!

@Zunawe Zunawe 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 May 23, 2024
@ThePhar ThePhar merged commit f40b10d into ArchipelagoMW:main Jun 1, 2024
16 checks passed
@Zunawe Zunawe deleted the emerald-option-descs branch June 1, 2024 22:05
wu4 pushed a commit to wu4/Archipelago that referenced this pull request Jun 6, 2024
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
sflavelle pushed a commit to sflavelle/Archipelago-tgc that referenced this pull request Jun 20, 2024
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: enhancement Issues requesting new features or pull requests implementing new features. 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
Development

Successfully merging this pull request may close these issues.

4 participants