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

A few minor tweaks now that we're on Python 3.8 #890

Merged
merged 10 commits into from
Jan 24, 2023
8 changes: 4 additions & 4 deletions .github/workflows/framework-tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:
uses: actions/setup-python@v2

- name: Install tox
run: pip install tox==4.2.8
run: pip install tox

- name: Run linting
run: tox -e lint
Expand All @@ -26,7 +26,7 @@ jobs:
uses: actions/setup-python@v2

- name: Install tox
run: pip install tox==4.2.8
run: pip install tox

- name: Run static type checks
run: tox -e static
Expand All @@ -47,7 +47,7 @@ jobs:
python-version: ${{ matrix.python-version }}

- name: Install tox
run: pip install tox==4.2.8
run: pip install tox

- name: Run unit tests
run: tox -e unit
Expand All @@ -73,7 +73,7 @@ jobs:
go-version: "1.17"

- name: Install tox
run: pip install tox==4.2.8
run: pip install tox

- name: Install Pebble
run: go install github.com/canonical/pebble/cmd/pebble@latest
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/observability-charm-tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
echo -e "\ngit+$GITHUB_SERVER_URL/$GITHUB_REPOSITORY@$GITHUB_SHA#egg=ops" >> requirements.txt

- name: Install dependencies
run: pip install tox==4.2.8
run: pip install tox

- name: Run the charm tests
run: tox -vve unit
98 changes: 47 additions & 51 deletions ops/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,30 +34,31 @@
from ops.framework import EventBase, EventSource, Framework, Object, ObjectEvents

if TYPE_CHECKING:
from typing_extensions import Literal, Required, TypedDict
from typing import Literal

from typing_extensions import Required, TypedDict

from ops.framework import Handle, JsonObject, _SerializedData
from ops.model import Container, Numerical, Relation, Storage

# CharmMeta also needs these.
_ActionParam = Dict[str, 'JsonObject'] # <JSON Schema definition>
_ActionMetaDict = TypedDict(
'_ActionMetaDict', {
'title': str,
'description': str,
'params': Dict[str, _ActionParam],
'required': List[str]},
total=False)

class _ActionMetaDict(TypedDict, total=False):
Copy link
Member

@jameinel jameinel Jan 17, 2023

Choose a reason for hiding this comment

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

This is the first that I've seen meta arguments to classes (total=False).
Apparently __init_subclass__ has been a thing since about python 3.6.

Copy link
Contributor

Choose a reason for hiding this comment

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

From the arg, I'd suspect that TypedDict is @dataclass, though I haven't actually looked at the class definition.

Do we have use for pseudo-metaclass stuff other than the __set_name__ changes from the last PR (part of the same PEP as __init_subclass__, IIRC)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Keep up with the times, @jameinel. ;-) Nah, seriously, Python moves way too fast these days IMO, particularly in the typing world.

title: str
description: str
params: Dict[str, _ActionParam]
required: List[str]

_Scopes = Literal['global', 'container']
_RelationMetaDict = TypedDict(
'_RelationMetaDict', {
'interface': Required[str],
'limit': int,
'scope': _Scopes},
total=False)

_MultipleRange = TypedDict('_MultipleRange', {'range': str})

class _RelationMetaDict(TypedDict, total=False):
interface: Required[str]
limit: int
scope: _Scopes

class _MultipleRange(TypedDict):
range: str
Copy link
Member

Choose a reason for hiding this comment

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

This looks weird. Why is _MultipleRange turned into a type, but _StorageMetaDict is not? Also, I think there should be another space here.

Note that I didn't try validating all the changes here, I just happened to see this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that was weird, wasn't it! It turned out it was because some are to define the shape of dicts that have "-" in their keys, like user-id and group-id, which of course can't be identifiers/class attributes. So those have to remain non-class syntax. I figured it was too confusing and weird to have both types, so I backed this change out.

Which doesn't leave much of a change, but oh well. :-)

_StorageMetaDict = TypedDict('_StorageMetaDict', {
'type': Required[str],
'description': int,
Expand All @@ -69,21 +70,20 @@
'multiple': _MultipleRange
})

_ResourceMetaDict = TypedDict(
'_ResourceMetaDict', {
'type': Required[str],
'filename': str,
'description': str},
total=False)
class _ResourceMetaDict(TypedDict, total=False):
type: Required[str]
filename: str
description: str

class _PayloadMetaDict(TypedDict):
type: str

_PayloadMetaDict = TypedDict('_PayloadMetaDict', {'type': str})
class _MountDict(TypedDict, total=False):
storage: Required[str]
location: str

_MountDict = TypedDict(
'_MountDict', {'storage': Required[str],
'location': str},
total=False)
_ContainerMetaDict = TypedDict(
'_ContainerMetaDict', {'mounts': List[_MountDict]})
class _ContainerMetaDict(TypedDict):
mounts: List[_MountDict]

_CharmMetaDict = TypedDict(
Copy link
Member

Choose a reason for hiding this comment

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

This should also be a class, vs a TypedDict instantiation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not quite sure what you mean here?

'_CharmMetaDict', { # all are optional
Expand All @@ -108,23 +108,20 @@
}, total=False)

# can't put in *Event because *Event.snapshot needs it.
_WorkloadEventSnapshot = TypedDict('_WorkloadEventSnapshot', {
'container_name': str
}, total=False)
class _WorkloadEventSnapshot(TypedDict, total=False):
container_name: str

_RelationDepartedEventSnapshot = TypedDict('_RelationDepartedEventSnapshot', {
'relation_name': str,
'relation_id': int,
'app_name': Optional[str],
'unit_name': Optional[str],
'departing_unit': Optional[str]
}, total=False)
class _RelationDepartedEventSnapshot(TypedDict, total=False):
relation_name: str
relation_id: int
app_name: Optional[str]
unit_name: Optional[str]
departing_unit: Optional[str]

_StorageEventSnapshot = TypedDict('_StorageEventSnapshot', {
'storage_name': str,
'storage_index': int,
'storage_location': str,
}, total=False)
class _StorageEventSnapshot(TypedDict, total=False):
storage_name: str
storage_index: int
storage_location: str


class HookEvent(EventBase):
Expand Down Expand Up @@ -425,12 +422,11 @@ class RelationEvent(HookEvent):

"""
if TYPE_CHECKING:
_RelationEventSnapshot = TypedDict('_RelationEventSnapshot', {
'relation_name': Required[str],
'relation_id': Required[int],
'app_name': Optional[str],
'unit_name': Optional[str]
}, total=False)
class _RelationEventSnapshot(TypedDict, total=False):
relation_name: Required[str]
relation_id: Required[int]
app_name: Optional[str]
unit_name: Optional[str]

def __init__(self, handle: 'Handle', relation: 'Relation',
app: Optional[model.Application] = None,
Expand Down Expand Up @@ -1195,7 +1191,7 @@ def __init__(self, role: RelationRole, relation_name: str, raw: '_RelationMetaDi
self.scope = raw.get('scope') or self._default_scope
if self.scope not in self.VALID_SCOPES:
raise TypeError("scope should be one of {}; not '{}'".format(
', '.join("'{}'".format(s) for s in self.VALID_SCOPES), self.scope))
', '.join(f"'{s}'" for s in self.VALID_SCOPES), self.scope))


class StorageMeta:
Expand Down
5 changes: 2 additions & 3 deletions ops/framework.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@

if TYPE_CHECKING:
from pathlib import Path

from typing_extensions import Literal, Protocol, Type
from typing import Literal, Protocol, Type

from ops.charm import CharmMeta
from ops.model import JsonObject, Model, _ModelBackend
Expand Down Expand Up @@ -613,7 +612,7 @@ def __init__(self, storage: Union[SQLiteStorage, JujuStorage],

# Parse the env var once, which may be used multiple times later
debug_at = os.environ.get('JUJU_DEBUG_AT')
self._juju_debug_at = (set(x.strip() for x in debug_at.split(','))
self._juju_debug_at = ({x.strip() for x in debug_at.split(',')}
Copy link
Member

Choose a reason for hiding this comment

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

I do wonder if this is actually better syntax :) But it exists, and I wouldn't block if someone was using it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, same. But speaking of better syntax, looking at it again I didn't like the nested inline if-else with set comprehension, so I've also split it onto multiple lines. Hopefully clearer now.

if debug_at else set()) # type: Set[str]

def set_breakpointhook(self):
Expand Down
2 changes: 1 addition & 1 deletion ops/lib/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ def _parse_lib(spec):
logger.debug(" Parsing %r", spec.name)

try:
with open(spec.origin, 'rt', encoding='utf-8') as f:
with open(spec.origin, encoding='utf-8') as f:
Copy link
Member

Choose a reason for hiding this comment

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

This feels like something that might be redundant with a default, but I don't mind being explicit to the developers as to expected behavior. Do you know why this was removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I guess pyupgrade removes it because it's the default (now? not sure how long that's been). Read/r has been the default forever, maybe the text/t default came later. But I can see where this is debatable, so I've reverted this change and just left it explicit.

libinfo = {}
for n, line in enumerate(f):
if len(libinfo) == len(_NEEDED_KEYS):
Expand Down
17 changes: 9 additions & 8 deletions ops/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@
Client,
ExecProcess,
FileInfo,
LayerDict,
Plan,
ServiceInfo,
_LayerDict,
)
from typing_extensions import TypedDict

Expand All @@ -87,10 +87,12 @@
# public since it is used in ops.testing
K8sSpec = Mapping[str, JsonObject]

_StatusDict = TypedDict('_StatusDict', {'status': str, 'message': str})
class _StatusDict(TypedDict):
status: str
message: str

# the data structure we can use to initialize pebble layers with.
_Layer = Union[str, LayerDict, pebble.Layer]
_Layer = Union[str, _LayerDict, pebble.Layer]

# mapping from relation name to a list of relation objects
_RelationMapping_Raw = Dict[str, Optional[List['Relation']]]
Expand All @@ -109,11 +111,10 @@
UnitOrApplication = Union['Unit', 'Application']
UnitOrApplicationType = Union[Type['Unit'], Type['Application']]

_AddressDict = TypedDict('_AddressDict', {
'address': str, # Juju < 2.9
'value': str, # Juju >= 2.9
'cidr': str
})
class _AddressDict(TypedDict):
address: str # Juju < 2.9
value: str # Juju >= 2.9
cidr: str
_BindAddressDict = TypedDict('_BindAddressDict', {
'interface-name': str,
'addresses': List[_AddressDict]
Expand Down
Loading