fix: use immutable types for state component attributes to reflect that they are immutable#2335
fix: use immutable types for state component attributes to reflect that they are immutable#2335tonyandrewmeyer wants to merge 2 commits intocanonical:mainfrom
Conversation
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
Changing from dict -> Mapping in the type annotations, and from set -> frozenset both in annotation and in actual code are technically not backwards compatible, though it should only be a problem if anyone is mutating their 'frozen' state. I'm in favour of making this change, but is there any due diligence we should do first?
| object.__setattr__(self, '_tracked_revision', 1) | ||
| object.__setattr__(self, '_latest_revision', 1) |
There was a problem hiding this comment.
These lines aren't necessary since we set the attributes directly in the class direclaration.
| object.__setattr__(self, 'rotate', rotate) | ||
| object.__setattr__(self, '_tracked_revision', 1) | ||
| object.__setattr__(self, '_latest_revision', 1) | ||
| _deepcopy_mutable_fields(self) |
There was a problem hiding this comment.
This should be unnecessary (and is inefficient) since we copy remote_grants manually above.
| remote_grants = {k: frozenset(v) for k, v in remote_grants.items()} | ||
| object.__setattr__(self, 'remote_grants', remote_grants) |
There was a problem hiding this comment.
I normally prefer the style that you have here, but I think we should stick to this pattern for consistency and clearer typing of the remote_grants variable.
| remote_grants = {k: frozenset(v) for k, v in remote_grants.items()} | |
| object.__setattr__(self, 'remote_grants', remote_grants) | |
| object.__setattr__( | |
| self, 'remote_grants', {k: frozenset(v) for k, v in remote_grants.items()} | |
| ) |
Can we run type checking (and unit tests) on the supertox charms? |
Yeah, I've done that before for other things. We also have the published charms list that get checked, although it still needs to love to handle the dropping of Python 3.8. The trickiest part is having to do many variants, because it might be "lint" or "static" or variants of those. |
|
Discussing with James whether to continue with this branch/PR or to revive the previous one. Maybe we want to do more than just the changes in here. |
3 participants
We expect all of the state attributes to be treated as immutable, but some are typed as
dict. Clean that up so that although the attribute may be adict, it is typed asMapping, so that a type checker will flag people wrongly mutating the state.Also adds a custom
__init__forSecret, which cleans up the private attributes (tracking revisions) and makes sure that theremote_grantsmapping has frozenset items rather than set.