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

refactor: cache signature structure in ops.testing state classes #1499

Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 2 additions & 2 deletions ops/_private/harness.py
Original file line number Diff line number Diff line change
Expand Up @@ -2271,13 +2271,13 @@ def _config_set(self, key: str, value: Union[str, int, float, bool]):
declared_type = option.get('type')
if not declared_type:
raise RuntimeError(
f'Incorrectly formatted `options.yaml`, option {key} '
f'Incorrectly formatted config in YAML, option {key} '
'is expected to declare a `type`.'
)

if declared_type not in self._supported_types:
raise RuntimeError(
'Incorrectly formatted `options.yaml`: `type` needs to be one '
'Incorrectly formatted config in YAML: `type` needs to be one '
'of [{}], not {}.'.format(', '.join(self._supported_types), declared_type)
)

Expand Down
4 changes: 3 additions & 1 deletion testing/src/scenario/_runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,9 @@ def _virtual_charm_root(self):
actions_yaml = virtual_charm_root / "actions.yaml"

metadata_files_present: Dict[Path, Optional[str]] = {
file: file.read_text() if file.exists() else None
file: file.read_text()
if charm_virtual_root_is_custom and file.exists()
else None
tonyandrewmeyer marked this conversation as resolved.
Show resolved Hide resolved
for file in (metadata_yaml, config_yaml, actions_yaml)
}

Expand Down
43 changes: 32 additions & 11 deletions testing/src/scenario/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,26 +122,43 @@ class _MaxPositionalArgs:

_max_positional_args = n

def __new__(cls, *args: Any, **kwargs: Any):
@classmethod
def _annotate_class(cls):
"""Record information about which parameters are positional vs. keyword-only."""
if hasattr(cls, "_init_parameters"):
# We don't support dynamically changing the signature of a
# class, so we assume here it's the same as previously.
tonyandrewmeyer marked this conversation as resolved.
Show resolved Hide resolved
# In addition, the class and the function that provides it
# are private, so we generally don't expect anyone to be
# doing anything radical with these.
return
# inspect.signature guarantees the order of parameters is as
# declared, which aligns with dataclasses. Simpler ways of
# getting the arguments (like __annotations__) do not have that
# guarantee, although in practice it is the case.
parameters = inspect.signature(cls.__init__).parameters
required_args = [
cls._init_parameters = parameters = inspect.signature(
cls.__init__
).parameters
cls._init_kw_only = {
name
for name in tuple(parameters)[cls._max_positional_args :]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this was auto-formatted by ruff, the dangling :] is kinda funny :]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was, yeah - I lazily just wrote it out on one line and let tox -e fmt clean up.

Does the trailing happy face bother you? I could do something like move tuple(parameters) out to a separate line to get the dict comprehension to be on a single line, if that would be better.

if not name.startswith("_")
}
cls._init_required_args = [
name
for name in tuple(parameters)
if parameters[name].default is inspect.Parameter.empty
and name not in kwargs
and name != "self"
if name != "self"
and parameters[name].default is inspect.Parameter.empty
]

def __new__(cls, *args: Any, **kwargs: Any):
cls._annotate_class()
required_args = [
name for name in cls._init_required_args if name not in kwargs
]
n_posargs = len(args)
max_n_posargs = cls._max_positional_args
kw_only = {
name
for name in tuple(parameters)[max_n_posargs:]
if not name.startswith("_")
}
kw_only = cls._init_kw_only
if n_posargs > max_n_posargs:
raise TypeError(
f"{cls.__name__} takes {max_n_posargs} positional "
Expand Down Expand Up @@ -180,6 +197,10 @@ def __reduce__(self):
return _MaxPositionalArgs


# A lot of JujuLogLine objects are created, so we want them to be fast and light.
# Dataclasses define __slots__, so are small, and a namedtuple is actually
# slower to create than a dataclass. A plain dictionary (or TypedDict) would be
# about twice as fast, but less convenient to use.
@dataclasses.dataclass(frozen=True)
class JujuLogLine:
"""An entry in the Juju debug-log."""
Expand Down
Loading