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

benhoyt · 2023-01-16T05:08:55Z

Originally this PR was running pyupgrade --py38-plus on the codebase, to pick up a few new features in Python 3.7 and 3.8 (apart from f-strings, which was done in #889). However, in particular we didn't like how it converted TypedDict to the class syntax, because then half of them were converted (the ones without - in the field names can't be converted). So avoid that so they're all consistent.

This includes:

Use of set literals and comprehensions instead of set(...)
Use OSError instead of IOError as the latter is now an alias
Use b'' instead of bytes()
One use of f-string that apparently flynt didn't pick up

This PR also:

Removes the tox 4.2.8 pinning now that Tox 4.3.0 breaks substitution for values from other sections tox-dev/tox#2863 is fixed.
Makes tox.ini source vars relative to fix autopep8 problem

This includes: * Use TypedDict classes to simplify type definitions * Use of "typing" module instead of "typing_extensions" where possible * Use of set literals and comprehensions instead of set(...) * Remove 't' arg to open() as it's the default with encoding * Use OSError instead of IOError as the latter is now an alias * Use b'' instead of bytes() * One use of f-string that apparently flynt didn't pick up Will follow up this commit with another on the same PR to fix styling.

Ref: tox-dev/tox#2863

jameinel

I'm not very sure about the TypedDict changes, and changing LayerDict to _LayerDict, I'd like to understand the rationale a bit better.

jameinel · 2023-01-17T15:35:22Z

ops/charm.py

+        scope: _Scopes
+
+    class _MultipleRange(TypedDict):
+        range: str


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.

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. :-)

jameinel · 2023-01-17T15:42:56Z

ops/charm.py

-            'required': List[str]},
-        total=False)
+
+    class _ActionMetaDict(TypedDict, total=False):


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.

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)?

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

jameinel · 2023-01-17T15:43:38Z

ops/charm.py

-    _ContainerMetaDict = TypedDict(
-        '_ContainerMetaDict', {'mounts': List[_MountDict]})
+    class _ContainerMetaDict(TypedDict):
+        mounts: List[_MountDict]

    _CharmMetaDict = TypedDict(


This should also be a class, vs a TypedDict instantiation

Not quite sure what you mean here?

jameinel · 2023-01-17T15:45:05Z

ops/framework.py

@@ -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(',')}


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

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.

jameinel · 2023-01-17T15:45:41Z

ops/lib/__init__.py

@@ -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:


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?

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.

jameinel · 2023-01-17T15:50:04Z

ops/pebble.py

+        http: Optional[_HttpDict]
+        tcp: Optional[_TcpDict]
+        exec: Optional[_ExecDict]
+        threshold: Optional[int]

    _AuthDict = TypedDict('_AuthDict',


I really don't understand how it found all the others, but keeps missing some of them.
Is there a special field that isn't supported by the class syntax? (user-id looks suspicious). Hm, that makes me wonder if the new syntax is actually better, or whether it just sets you up to think of things in terms of a Type, when it really is just a dict. (it is far fewer quotes and commas, but that seems to be the only real difference)

I haven't gone through an automated process that closely -- iterative improvements will take a bit, and it's easier (from my POV) to take a single "I ran this tool" PR and merge it into main with a smaller PR to reconcile what it may have missed later than to line-item them.

In general, tend to look at stuff like this and say "well, since Python is still pretty dependent on the exact ordering of class/variable definition in a single file in 2023, maybe converting this particular one to a class would result in some impossible intermediate bytecode". I don't know why a fully-completed class definition if (if this were just class _AuthDict(TypedDict, ...) wouldn't have finished bytecode evaluation by then, but it's sometimes surprisingly fragile.

It doesn't hurt to see what happens if it's replaced by hand, but I'd also expect the tool to be a little conservative.

@jameinel See above: https://github.com/canonical/operator/pull/890/files/49e6c623ecc2802d141f4420ff8ffb30641b05aa#r1082094668

jameinel · 2023-01-17T15:54:51Z

ops/pebble.py

@@ -702,12 +712,12 @@ class Layer:
    #: Mapping of check to :class:`Check` defined by this layer.
    checks: Dict[str, 'Check']

-    def __init__(self, raw: Optional[Union[str, 'LayerDict']] = None):
+    def __init__(self, raw: Optional[Union[str, '_LayerDict']] = None):


Given the number of references to LayerDict, why are we making it 'private' ? I'm not sure if any of these should be prefixed with _ if the intent is that people are going to use them outside of that module.

I actually really like this change. The Python ecosystem is always frantic, and DeepDiff requires a Rust compiler as of... sometime last week. "Is this dict equal to that dict?" gets really snarly when there are dicts nested under it, and encouraging to avoid it by making it more annoying is a plus from my point of view.

In addition, the fields under it are comprised of dunder types anyway.

I wanted to make it private because I don't really want us to commit to the type annotations being stable just yet. But it's unnecessary churn and could potentially break people (even if only typing it doesn't seem worth it). So I've reversed that part of the change.

…fixes" This reverts commit b2f6832.

This reverts commit 8bf0b57.

This excludes: * The TypedDict changes, only half of which could be converted to the class-based syntax anyway, due to many keys having '-' in them, like "user-id". So keep them consistent. * Keep the "rt" and similar mode arguments to open() to be explicit, even when they're default. Also simplify the nested inline if-else with set comprehension for _juju_debug_at into multiple lines -- simpler and clearer.

benhoyt added 4 commits January 16, 2023 17:59

LayerDict -> _LayerDict; RawConfig -> _RawConfig; formatting fixes

b2f6832

Make tox.ini source vars relative to fix autopep8 problem

10f995d

Remove tox==4.2.8 pinning now that Tox bug has been fixed

49e6c62

Ref: tox-dev/tox#2863

benhoyt requested review from rbarry82 and jameinel January 16, 2023 05:09

rbarry82 approved these changes Jan 16, 2023

View reviewed changes

jameinel reviewed Jan 17, 2023

View reviewed changes

benhoyt added 6 commits January 19, 2023 17:41

Use *.py in pyright include spec; fix yaml.py annotations

5dd89a4

Revert "LayerDict -> _LayerDict; RawConfig -> _RawConfig; formatting …

aef2f55

…fixes" This reverts commit b2f6832.

Revert "Run "pyupgrade --py38-plus" on codebase"

c147a57

This reverts commit 8bf0b57.

Merge branch 'main' into pyupgrade

f26c710

Merge branch 'main' into pyupgrade

1bccced

benhoyt changed the title ~~Run "pyupgrade --py38-plus" on the codebase~~ A few minor tweaks now that we're on Python 3.8 Jan 20, 2023

benhoyt requested review from jameinel and rbarry82 January 20, 2023 04:16

jameinel approved these changes Jan 24, 2023

View reviewed changes

benhoyt merged commit b1ed62e into canonical:main Jan 24, 2023

benhoyt deleted the pyupgrade branch January 24, 2023 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

benhoyt commented Jan 16, 2023 •

edited

jameinel left a comment

jameinel Jan 17, 2023

benhoyt Jan 20, 2023

jameinel Jan 17, 2023 •

edited by benhoyt

rbarry82 Jan 17, 2023

benhoyt Jan 20, 2023

jameinel Jan 17, 2023

benhoyt Jan 20, 2023

jameinel Jan 17, 2023

benhoyt Jan 20, 2023

jameinel Jan 17, 2023

benhoyt Jan 20, 2023

jameinel Jan 17, 2023

rbarry82 Jan 17, 2023

benhoyt Jan 20, 2023

jameinel Jan 17, 2023

rbarry82 Jan 17, 2023

benhoyt Jan 20, 2023

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

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

Conversation

benhoyt commented Jan 16, 2023 • edited

jameinel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameinel Jan 17, 2023 • edited by benhoyt

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt commented Jan 16, 2023 •

edited

jameinel Jan 17, 2023 •

edited by benhoyt