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: Make csv options output ignore hidden options #4539

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

Conversation

Jarno458
Copy link
Collaborator

What is this fixing or adding?

Generate with --csv_output would also output options set to Visibility.none, this causes an issue in for example Timespinner's backwards compatibility options, causing their values to display rather then new options as they share the same display name

How was this tested?

locally generating with --csv_output and with and without this change

@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 Jan 22, 2025
@ScipioWright ScipioWright added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label Jan 22, 2025
@Exempt-Medic
Copy link
Member

Exempt-Medic commented Jan 22, 2025

Somewhat unrelated, but why do the invisible options have display names?

Options.py Outdated Show resolved Hide resolved
@Berserker66
Copy link
Member

Would it make sense to read the spoiler log flag for outputting to csv_output?

Options.py Outdated
@@ -1582,7 +1582,7 @@ def dump_player_options(multiworld: MultiWorld) -> None:
}
output.append(player_output)
for option_key, option in world.options_dataclass.type_hints.items():
if issubclass(Removed, option):
if issubclass(Removed, option) or option.visibility == Visibility.none:
Copy link
Member

@Berserker66 Berserker66 Jan 22, 2025

Choose a reason for hiding this comment

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

Suggested change
if issubclass(Removed, option) or option.visibility == Visibility.none:
if Visibility.spoiler not in option.visibility:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i dunno maybe

Copy link
Member

@Exempt-Medic Exempt-Medic Jan 22, 2025

Choose a reason for hiding this comment

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

I think you'd still want non-spoiler options. Like if a world displayed an option in its own way and set it to not appear in the spoiler, it could still impact logic and be valuable to the csv output

Co-authored-by: Aaron Wagener <[email protected]>
Copy link
Collaborator

@qwint qwint left a comment

Choose a reason for hiding this comment

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

pure code review

change seems reasonable, code seems correct, my only worry would be if there are any Removed options that override visibility that now get added to the output when they wouldn't before
but i'm not sure what the usecase for that would be

@alwaysintreble alwaysintreble 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 Jan 29, 2025
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. 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
Development

Successfully merging this pull request may close these issues.

6 participants