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

HK: add location counts to option descriptions #4083

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

qwint
Copy link
Contributor

@qwint qwint commented Oct 22, 2024

What is this fixing or adding?

adds auto generated location counts to options that update locations by extracted data

How was this tested?

lightly, looking at webhost and generated template yaml to make sure they read fine

If this makes graphical changes, please attach screenshots.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Oct 22, 2024
@@ -132,7 +132,8 @@
for option_name, option_data in pool_options.items():
extra_data = {"__module__": __name__, "items": option_data[0], "locations": option_data[1]}
if option_name in option_docstrings:
extra_data["__doc__"] = option_docstrings[option_name]
extra_data["__doc__"] = option_docstrings[option_name] + \
f"\n This option adds {len(option_data[0])} location(s)"
Copy link
Member

Choose a reason for hiding this comment

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

You could do this (also adds a missing final period):

Suggested change
f"\n This option adds {len(option_data[0])} location(s)"
f"\n This option adds {len(option_data[0])} location{'' if len(option_data[0]) == 1 else 's'}."

@Exempt-Medic
Copy link
Member

Exempt-Medic commented Oct 22, 2024

The changes LGTM overall. One comments on the "location(s)" made earlier, other than that Elevator Pass doesn't list a value and Nail claims it adds four locations when it's actually three. Split items also aren't listed but that seems fine.

@Exempt-Medic Exempt-Medic added is: enhancement Issues requesting new features or pull requests implementing new features. is: documentation Improvements or additions to documentation. waiting-on: author Issue/PR is waiting for feedback or changes from its author. labels Oct 22, 2024
@qwint
Copy link
Contributor Author

qwint commented Oct 27, 2024

between this and #4079 whichever doesn't get merged first needs to update the other

@Exempt-Medic Exempt-Medic removed the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Oct 28, 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.

Oops never hit the approve button

@Nidoskull
Copy link

Not a code reviewer, but if it works I'm very pro-adding this. Location count is very useful for organizing games, and knowing roughly how many locations a yaml adds without having to generate first is very useful for Joining games 👍

@qwint
Copy link
Contributor Author

qwint commented Dec 5, 2024

@BadMagic100 (since you asked for it)

Copy link
Collaborator

@BadMagic100 BadMagic100 left a comment

Choose a reason for hiding this comment

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

Light nitpicky suggestion, perhaps "approximately x locations" is better, to set the expectation the exact location count may differ (eg excluded palace, variation based on item count and such). Otherwise looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: documentation Improvements or additions to documentation. is: enhancement Issues requesting new features or pull requests implementing new features. 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.

4 participants