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

Core, Webhost, Docs: Replace all usages of player settings #3067

Merged

Conversation

nicholassaylor
Copy link
Contributor

@nicholassaylor nicholassaylor commented Mar 31, 2024

What is this fixing or adding?

  1. Replaces the remaining usages of player settings with player options

  2. Changes Generate.py to reference player options, changing the --samesettings argument to --sameoptions

  3. Removed backwards compatability for player-settings link

    • /player-settings links should no longer auto-redirect to /player-options page

How was this tested?

  1. Searched for all instances of "settings" or "player-settings" and replaced any that did not refer to the Settings API
  2. Looked at rendered markdown
  3. Ran Generate.py with --sameoptions argument and got expected output
    • Would appreciate if another person could test argument change to make sure that I did everything correct (first time touching core)

@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 Mar 31, 2024
@Exempt-Medic
Copy link
Member

Exempt-Medic commented Mar 31, 2024

There's also a line in here you'll want to change: https://github.com/ArchipelagoMW/Archipelago/blob/main/worlds/archipidle/docs/guide_en.md
Specifically "setting page above"

@nicholassaylor
Copy link
Contributor Author

There's also a line in here you'll want to change: https://github.com/ArchipelagoMW/Archipelago/blob/main/worlds/archipidle/docs/guide_en.md Specifically "setting page above"

Thank you, it took me a few reads to see where it was

@Exempt-Medic
Copy link
Member

Exempt-Medic commented Apr 1, 2024

You can probably also update these:

@nicholassaylor
Copy link
Contributor Author

You can probably also update these:

* https://github.com/ArchipelagoMW/Archipelago/blob/main/WebHostLib/misc.py#L40-L43

* https://github.com/ArchipelagoMW/Archipelago/blob/main/WebHostLib/misc.py#L52-L55
  Hmm, actually, looking at instances of `weighted-settings` that one probably can't be changed yet

Ahh, I didn't notice that there was specific backwards compat with weighted right now. I'll look into and see what I can find out. This PR can be marked as waiting on: author for now until I figure out the WebHost-specific changes I would need

@Exempt-Medic
Copy link
Member

Ahh, I didn't notice that there was specific backwards compat with weighted right now. I'll look into and see what I can find out. This PR can be marked as waiting on: author for now until I figure out the WebHost-specific changes I would need

I think it'd be fine if this PR just did everything for player-settings. The weighted page should probably be left for later anyway

@nicholassaylor
Copy link
Contributor Author

nicholassaylor commented Apr 1, 2024

I think it'd be fine if this PR just did everything for player-settings. The weighted page should probably be left for later anyway

I suppose that works, especially with the revamped options pages in the works.

@ScipioWright
Copy link
Collaborator

I see newlines were added in several of these.
I know for option descriptions, newlines are taken into account and, in some cases, make it look weird on the website.
Did you check to make sure that doesn't happen with these pages too?

@nicholassaylor
Copy link
Contributor Author

I can double check, all of those newline were added in by PyCharm automatically

@alwaysintreble
Copy link
Collaborator

unsure if it's in scope for this PR but there are some remaining f strings in Generate.py that still reference player settings

@LegendaryLinux
Copy link
Member

Thus far this does not conflict with the option-gruops PR, so it has my approval pending any further changes.

@nicholassaylor nicholassaylor changed the title Docs: Replace all usages of player settings Core, Docs: Replace all usages of player settings Apr 4, 2024
@nicholassaylor
Copy link
Contributor Author

unsure if it's in scope for this PR but there are some remaining f strings in Generate.py that still reference player settings

I did my best to address the Generate.py f strings and arguments. I think I caught everything in there

I see newlines were added in several of these. I know for option descriptions, newlines are taken into account and, in some cases, make it look weird on the website. Did you check to make sure that doesn't happen with these pages too?

I reverted all of the changes that were just newlines put in automatically. None of the files should have different newline behaviors.

@Exempt-Medic
Copy link
Member

Not really sure what your Discord name is, if anything, but I opened a PR on your fork with two suggested changes

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.

Two small things that stuck out. Everything else looks good

worlds/archipidle/docs/guide_fr.md Show resolved Hide resolved
@@ -103,8 +103,6 @@ shuffle_structures:
off: 0
```

För mer detaljer om vad varje inställning gör, kolla standardinställningen `PlayerSettings.yaml` som kommer med
Copy link
Member

Choose a reason for hiding this comment

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

You removed this paragraph for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This paragraph references the PlayerSettings.yaml file that no longer exists as of #3062 . The document overall is very outdated, but I don't know any Swedish to update it.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me then!

One missed "setting", removing a fallback
@github-actions github-actions bot added the affects: webhost Issues/PRs that touch webhost and may need additional validation. label Apr 5, 2024
Co-authored-by: Exempt-Medic <[email protected]>
@nicholassaylor nicholassaylor changed the title Core, Docs: Replace all usages of player settings Core, Webhost, Docs: Replace all usages of player settings Apr 5, 2024
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.

All the changes LGTM. Searched for other instances of "settings" in the options sense and didn't find any

@LegendaryLinux LegendaryLinux merged commit 569c37c into ArchipelagoMW:main Apr 6, 2024
15 checks passed
@nicholassaylor nicholassaylor deleted the docs-settings-cleanup branch April 7, 2024 00:24
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
…goMW#3067)

* Replace all usages of player settings

* Fixed line break error

* Attempt to fix line break again

* Finally figure out what Pycharm did to this file

* Pycharm search failed me

* Remove duplicate s

* Update ArchipIdle

* Revert random newline changes from Pycharm

* Remove player settings from fstrings and rename --samesettings to --sameoptions

* Finally get PyCharm to not auto-format my commits, randomly inserting the newlines

* Removing player-settings

* Missed one

* Remove final line break error
Co-authored-by: Exempt-Medic <[email protected]>

---------

Co-authored-by: Exempt-Medic <[email protected]>
Co-authored-by: Exempt-Medic <[email protected]>
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
…goMW#3067)

* Replace all usages of player settings

* Fixed line break error

* Attempt to fix line break again

* Finally figure out what Pycharm did to this file

* Pycharm search failed me

* Remove duplicate s

* Update ArchipIdle

* Revert random newline changes from Pycharm

* Remove player settings from fstrings and rename --samesettings to --sameoptions

* Finally get PyCharm to not auto-format my commits, randomly inserting the newlines

* Removing player-settings

* Missed one

* Remove final line break error
Co-authored-by: Exempt-Medic <[email protected]>

---------

Co-authored-by: Exempt-Medic <[email protected]>
Co-authored-by: Exempt-Medic <[email protected]>
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
…goMW#3067)

* Replace all usages of player settings

* Fixed line break error

* Attempt to fix line break again

* Finally figure out what Pycharm did to this file

* Pycharm search failed me

* Remove duplicate s

* Update ArchipIdle

* Revert random newline changes from Pycharm

* Remove player settings from fstrings and rename --samesettings to --sameoptions

* Finally get PyCharm to not auto-format my commits, randomly inserting the newlines

* Removing player-settings

* Missed one

* Remove final line break error
Co-authored-by: Exempt-Medic <[email protected]>

---------

Co-authored-by: Exempt-Medic <[email protected]>
Co-authored-by: Exempt-Medic <[email protected]>
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: 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants