Skip to content

Commit

Permalink
A few minor tweaks now that we're on Python 3.8 (#890)
Browse files Browse the repository at this point in the history
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-dev/tox#2863 is fixed.
* Makes tox.ini source vars relative to fix autopep8 problem
  • Loading branch information
benhoyt authored Jan 24, 2023
1 parent 7faa5a2 commit b1ed62e
Show file tree
Hide file tree
Showing 11 changed files with 26 additions and 23 deletions.
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 @@ -25,7 +25,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
6 changes: 3 additions & 3 deletions ops/_private/yaml.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@
_safe_dumper = getattr(yaml, 'CSafeDumper', yaml.SafeDumper)


def safe_load(stream: Union[str, TextIO]):
def safe_load(stream: Union[str, TextIO]) -> Any:
"""Same as yaml.safe_load, but use fast C loader if available."""
return yaml.load(stream, Loader=_safe_loader)
return yaml.load(stream, Loader=_safe_loader) # type: ignore


@overload
Expand All @@ -40,4 +40,4 @@ def safe_dump(data: Any, stream: Optional[Union[str, TextIO]] = None, **kwargs:
If `encoding:str` is provided, return bytes. Else, return str.
"""
return yaml.dump(data, stream=stream, Dumper=_safe_dumper, **kwargs)
return yaml.dump(data, stream=stream, Dumper=_safe_dumper, **kwargs) # type: ignore
2 changes: 1 addition & 1 deletion ops/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -1195,7 +1195,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
9 changes: 5 additions & 4 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,8 +612,10 @@ 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(','))
if debug_at else set()) # type: Set[str]
if debug_at:
self._juju_debug_at = {x.strip() for x in debug_at.split(',')}
else:
self._juju_debug_at: Set[str] = set()

def set_breakpointhook(self):
"""Hook into sys.breakpointhook so the builtin breakpoint() works as expected.
Expand Down
2 changes: 1 addition & 1 deletion ops/pebble.py
Original file line number Diff line number Diff line change
Expand Up @@ -1486,7 +1486,7 @@ def _request_raw(
try:
body = json.loads(e.read()) # type: Dict[str, Any]
message = body['result']['message'] # type: str
except (IOError, ValueError, KeyError) as e2:
except (OSError, ValueError, KeyError) as e2:
# Will only happen on read error or if Pebble sends invalid JSON.
body = {} # type: Dict[str, Any]
message = f'{type(e2).__name__} - {e2}'
Expand Down
6 changes: 3 additions & 3 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ per-file-ignores = ["test/*:D100,D101,D102,D103,D104"]
docstring-convention = "google"

[tool.pyright]
include = ["ops/jujuversion.py", "ops/log.py", "ops/model.py", "ops/version.py",
"ops/__init__.py", "ops/framework.py", "ops/pebble.py", "ops/charm.py",
"ops/storage.py", "ops/main.py", "ops/testing.py", "ops/_private/timeconv.py"]
include = ["ops/*.py", "ops/_private/*.py"]
pythonVersion = "3.8" # check no python > 3.8 features are used
pythonPlatform = "All"
typeCheckingMode = "strict"
reportIncompatibleMethodOverride = false
reportImportCycles = false
reportTypeCommentUsage = false
reportMissingModuleSource = false
stubPath = ""
2 changes: 1 addition & 1 deletion test/pebble_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ def main():
try:
with open(args.layer_path, encoding='utf-8') as f:
layer_yaml = f.read()
except IOError as e:
except OSError as e:
parser.error(f'cannot read layer YAML: {e}')
client.add_layer(args.label, layer_yaml, combine=bool(args.combine))
result = f'Layer {args.label!r} added successfully from {args.layer_path!r}'
Expand Down
4 changes: 2 additions & 2 deletions test/test_framework.py
Original file line number Diff line number Diff line change
Expand Up @@ -1233,11 +1233,11 @@ def test_mutable_types(self):
), (
lambda: set(),
'a',
set(['a']),
{'a'},
lambda a, b: a.add(b),
lambda res, expected_res: self.assertEqual(res, expected_res)
), (
lambda: set(['a']),
lambda: {'a'},
'a',
set(),
lambda a, b: a.discard(b),
Expand Down
2 changes: 1 addition & 1 deletion test/test_pebble.py
Original file line number Diff line number Diff line change
Expand Up @@ -1178,7 +1178,7 @@ def test_multipart_parser(self):

def handle_header(data):
headers.append(bytes(data))
bodies.append(bytes())
bodies.append(b'')
bodies_done.append(False)

def handle_body(data, done=False):
Expand Down
6 changes: 4 additions & 2 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ skip_missing_interpreters = True
envlist = lint, unit

[vars]
src_path = {toxinidir}/ops/
tst_path = {toxinidir}/test/
# These need to be relative paths because autopep8 doesn't handle absolute
# paths with relative paths in "exclude" correctly.
src_path = ops/
tst_path = test/
all_path = {[vars]src_path} {[vars]tst_path}

[testenv]
Expand Down

0 comments on commit b1ed62e

Please sign in to comment.