Skip to content

Core: typing for Option.default and a few other ClassVars #2899

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

Merged
merged 7 commits into from
Mar 12, 2024
Merged
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions Options.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ def __new__(mcs, name, bases, attrs):
aliases = {name[6:].lower(): option_id for name, option_id in attrs.items() if
name.startswith("alias_")}

assert (
name in {"Option", "VerifyKeys"} or # base abstract classes don't need default
"default" in attrs or
any(hasattr(base, "default") for base in bases)
), f"Option class {name} needs default value"
assert "random" not in aliases, "Choice option 'random' cannot be manually assigned."

# auto-alias Off and On being parsed as True and False
Expand Down Expand Up @@ -96,7 +101,7 @@ def meta__init__(self, *args, **kwargs):

class Option(typing.Generic[T], metaclass=AssembleOptions):
value: T
default = 0
default: typing.ClassVar[typing.Any] # something that __init__ will be able to convert to the correct type
Copy link
Contributor

@Jouramie Jouramie Mar 6, 2024

Choose a reason for hiding this comment

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

It feels counterintuitive to a different type for the default than T, but since most default values are immutable or literal "random" whereas T is not, I think it makes sense. 👍


# convert option_name_long into Name Long as display_name, otherwise name_long is the result.
# Handled in get_option_name()
Expand All @@ -106,8 +111,9 @@ class Option(typing.Generic[T], metaclass=AssembleOptions):
supports_weighting = True

# filled by AssembleOptions:
name_lookup: typing.Dict[T, str]
options: typing.Dict[str, int]
name_lookup: typing.ClassVar[typing.Dict[T, str]] # type: ignore
# https://github.com/python/typing/discussions/1460 the reason for this type: ignore
options: typing.ClassVar[typing.Dict[str, int]]

def __repr__(self) -> str:
return f"{self.__class__.__name__}({self.current_option_name})"
Expand Down Expand Up @@ -160,6 +166,8 @@ class FreeText(Option[str]):
"""Text option that allows users to enter strings.
Needs to be validated by the world or option definition."""

default = ""

def __init__(self, value: str):
assert isinstance(value, str), "value of FreeText must be a string"
self.value = value
Expand Down Expand Up @@ -803,7 +811,7 @@ def verify(self, world: typing.Type[World], player_name: str, plando_options: "P


class OptionDict(Option[typing.Dict[str, typing.Any]], VerifyKeys, typing.Mapping[str, typing.Any]):
default: typing.Dict[str, typing.Any] = {}
default = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I get that the default value was not changed, only the type removed, but shouldn't that default default value be something immutable? For safety measure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It has been considered and discussed in the past. If it were as simple and straightforward as OptionList and OptionSet, I think it would have been done before.
But yeah, that's out of scope for this PR.

supports_weighting = False

def __init__(self, value: typing.Dict[str, typing.Any]):
Expand Down Expand Up @@ -844,7 +852,7 @@ class OptionList(Option[typing.List[typing.Any]], VerifyKeys):
# If only unique entries are needed and input order of elements does not matter, OptionSet should be used instead.
# Not a docstring so it doesn't get grabbed by the options system.

default: typing.Union[typing.List[typing.Any], typing.Tuple[typing.Any, ...]] = ()
default = ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep the more precises types? This could be typing.ClassVar[typing.Iterable[str]] for instance?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm, I should have read #2173 before commenting. All good 👍

supports_weighting = False

def __init__(self, value: typing.Iterable[typing.Any]):
Expand All @@ -870,7 +878,7 @@ def __contains__(self, item):


class OptionSet(Option[typing.Set[str]], VerifyKeys):
default: typing.Union[typing.Set[str], typing.FrozenSet[str]] = frozenset()
default = frozenset()
supports_weighting = False

def __init__(self, value: typing.Iterable[str]):
Expand Down