Add _(to|from)_json() methods to all option types #11
Conversation
options.py
Outdated
| @@ -81,6 +89,9 @@ class ValueOption[J: JSON](BaseOption): | |||
| def __init__(self) -> None: | |||
| raise NotImplementedError | |||
|
|
|||
| def _to_json(self) -> JSON: | |||
| return cast(JSON, self.value) | |||
There was a problem hiding this comment.
is this cast actually necessary? self.value should be json already, surely you can widen it.
also as a nitpick, a cast has a performance impact, it does an actual function call. if you don't need the result straight away (e.g. cause you're returning it), just a type ignore is slightly better
| @@ -147,6 +158,9 @@ class HiddenOption[J: JSON](ValueOption[J]): | |||
| init=False, | |||
| ) | |||
|
|
|||
| def _from_json(self, value: JSON) -> None: | |||
| self.value = cast(J, value) | |||
There was a problem hiding this comment.
in this case I get the cast since it's narrowing
options.py
Outdated
| def _to_json(self) -> JSON: | ||
| raise NotImplementedError("_to_json should never be called on a ButtonOption") | ||
|
|
||
| def _from_json(self, value: JSON) -> None: | ||
| raise NotImplementedError("_from_json should never be called on a ButtonOption") |
There was a problem hiding this comment.
This is kind of awkward, if there's one specific type you always need to special case. You run into that problem further down in fact.
What if to json returned ellipsis, to mark it as unused, and from was just a noop?
There was a problem hiding this comment.
this is the main one, rest is nitpicks/consequences of this
settings.py
Outdated
| # Button option is the only standard option which is not abstract, but also not a value, | ||
| # and doesn't have children. | ||
| # Just no-op it so that it doesn't show an error | ||
| if isinstance(option, ButtonOption): | ||
| continue | ||
| settings[option.identifier] = option._to_json() # type: ignore |
There was a problem hiding this comment.
should be able to turn this into a dict comprehension once the issue with button is solved
options.py
Outdated
| @abstractmethod | ||
| def _to_json(self) -> JSON: | ||
| raise NotImplementedError | ||
|
|
||
| @abstractmethod | ||
| def _from_json(self, value: JSON) -> None: | ||
| raise NotImplementedError |
There was a problem hiding this comment.
this is still somewhat of an interface, so good to add docstrings - especially to explain ellipsis
Turns this option into a JSON value.
Returns:
This option's JSON representation, or Ellipsis if it should be considered to have no value.
Assigns this option's value, based on a previously retrieved JSON value.
Args:
value: The JSON value to assign.
|
I made the requested changes. Played around with returning a sentinel that wouldn't break serialization instead of Ellipsis, but gave up on it due to added confusion. Probably easier to communicate what's going on through the typing system. BaseOption._to_json() returns JSON | EllipsisType, so trying to assign that value to Mapping[str, JSON] gives a warning unless you handle it explicitly. |
Adding helpers to all option types for easier saving to settings files. Will allow the in-development save_options module to handle serialization in a much cleaner way.
Two substantive updates: