From 1836df5affb42b3183125b1904c794090aa1862b Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 16 Feb 2024 08:35:48 +1300 Subject: [PATCH] chore: expand ruff rule sets and make corresponding minor adjustments (#1114) This PR enables a few additional `ruff` checks - since they are bundled with `ruff` and `ruff` is extremely fast, they are basically free. I cherry-picked the set that I agree with/like the most - I'm happy to argue for the inclusion of any specific ones where you disagree, or consider [other rules](https://docs.astral.sh/ruff/rules/) if you have preferences. * pyupgrade (UP) - I love this tool - I find it's a super easy way to adopt new features as you move up your minimum-supported Python version (in using it for a year or so I haven't ever had it suggest something that I didn't like) * flake8-2020 (YTT) * bandit (S) - a good tool for avoiding accidentally missing things, in my opinion (marking them with `noqa` shows that thought has gone into it, which has value in itself) * flake8-bugbear (B) * flake8-simplify (SIM) * Ruff specific (RUF) - seems like we should since we're using the tool * perflint (PERF) - seems to make good suggestions in general Notes: Where the linter picks up new issues, these are handled in one of these ways: * Ignore with a `noqa:` directive if it's a false positive or should otherwise be permanently ignored in that specific case * Ignore for a file or group of files (the docs and tests have several of these) where it's something we want to pick up in the core code but not everywhere * Ignore with a note to review later when it's likely that there would be too much additional noise in this PR * Make the recommended change, when it's small and seems reasonable Continues from #1102. --- ops/__init__.py | 2 +- ops/_private/yaml.py | 2 +- ops/charm.py | 22 +++-- ops/lib/__init__.py | 8 +- ops/main.py | 5 +- ops/model.py | 36 ++++---- ops/pebble.py | 128 +++++++++++++++-------------- ops/storage.py | 4 +- ops/testing.py | 98 ++++++++++------------ pyproject.toml | 76 ++++++++++++++++- test/charms/test_main/src/charm.py | 2 +- test/pebble_cli.py | 10 +-- test/test_helpers.py | 2 +- test/test_infra.py | 2 +- test/test_lib.py | 6 +- test/test_main.py | 2 +- test/test_pebble.py | 13 +-- test/test_private.py | 2 +- test/test_real_pebble.py | 8 +- test/test_storage.py | 6 +- test/test_testing.py | 12 +-- tox.ini | 2 +- 22 files changed, 251 insertions(+), 197 deletions(-) diff --git a/ops/__init__.py b/ops/__init__.py index 4306ee60a..2f359c2b0 100644 --- a/ops/__init__.py +++ b/ops/__init__.py @@ -41,7 +41,7 @@ # The "from .X import Y" imports below don't explicitly tell Pyright (or MyPy) # that those symbols are part of the public API, so we have to add __all__. -__all__ = [ +__all__ = [ # noqa: RUF022 `__all__` is not sorted '__version__', 'main', 'pebble', diff --git a/ops/_private/yaml.py b/ops/_private/yaml.py index 78b28533f..1ae0f8826 100644 --- a/ops/_private/yaml.py +++ b/ops/_private/yaml.py @@ -25,7 +25,7 @@ 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) # type: ignore + return yaml.load(stream, Loader=_safe_loader) # type: ignore # noqa: S506 def safe_dump(data: Any, stream: Optional[TextIO] = None) -> str: diff --git a/ops/charm.py b/ops/charm.py index 8b2586d3c..99e1f2651 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -242,9 +242,9 @@ class StartEvent(HookEvent): This event is triggered immediately after the first :class:`ConfigChangedEvent`. Callback methods bound to the event should be - used to ensure that the charm’s software is in a running state. Note that - the charm’s software should be configured so as to persist in this state - through reboots without further intervention on Juju’s part. + used to ensure that the charm's software is in a running state. Note that + the charm's software should be configured so as to persist in this state + through reboots without further intervention on Juju's part. """ @@ -253,8 +253,8 @@ class StopEvent(HookEvent): This event is triggered when an application's removal is requested by the client. The event fires immediately before the end of the - unit’s destruction sequence. Callback methods bound to this event - should be used to ensure that the charm’s software is not running, + unit's destruction sequence. Callback methods bound to this event + should be used to ensure that the charm's software is not running, and that it will not start again on reboot. """ @@ -545,7 +545,7 @@ class RelationChangedEvent(RelationEvent): are incomplete, since it can be guaranteed that when the remote unit or application changes its settings, the event will fire again. - The settings that may be queried, or set, are determined by the relation’s + The settings that may be queried, or set, are determined by the relation's interface. """ @@ -560,8 +560,8 @@ class RelationDepartedEvent(RelationEvent): emitted once for each remaining unit. Callback methods bound to this event may be used to remove all - references to the departing remote unit, because there’s no - guarantee that it’s still part of the system; it’s perfectly + references to the departing remote unit, because there's no + guarantee that it's still part of the system; it's perfectly probable (although not guaranteed) that the system running that unit has already shut down. @@ -619,7 +619,7 @@ class RelationBrokenEvent(RelationEvent): fire to signal that the relationship has been fully terminated. The event indicates that the current relation is no longer valid, and that - the charm’s software must be configured as though the relation had never + the charm's software must be configured as though the relation had never existed. It will only be called after every callback method bound to :class:`RelationDepartedEvent` has been run. If a callback method bound to this event is being executed, it is guaranteed that no remote units @@ -676,8 +676,7 @@ def restore(self, snapshot: Dict[str, Any]): if storage_location is None: raise RuntimeError( 'failed loading storage location from snapshot.' - '(name={!r}, index={!r}, storage_location=None)' - .format(storage_name, storage_index)) + f'(name={storage_name!r}, index={storage_index!r}, storage_location=None)') self.storage.location = storage_location @@ -1007,7 +1006,6 @@ def _on_collect_status(self, event: ops.CollectStatusEvent): event.add_status(ops.BlockedStatus('please set "port" config')) return event.add_status(ops.ActiveStatus()) - """ # noqa: D405, D214, D411, D416 Final return confuses docstyle. def add_status(self, status: model.StatusBase): diff --git a/ops/lib/__init__.py b/ops/lib/__init__.py index 2d201256d..dde350159 100644 --- a/ops/lib/__init__.py +++ b/ops/lib/__init__.py @@ -32,7 +32,7 @@ from types import ModuleType from typing import List -__all__ = ('use', 'autoimport') +__all__ = ('autoimport', 'use') logger = logging.getLogger(__name__) @@ -182,7 +182,7 @@ def _join_and(keys: List[str]) -> str: class _Missing: """Helper to get the difference between what was found and what was needed when logging.""" - def __init__(self, found): + def __init__(self, found: bool): self._found = found def __str__(self): @@ -202,7 +202,7 @@ def _parse_lib(spec: ModuleSpec) -> typing.Optional["_Lib"]: 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: libinfo = {} for n, line in enumerate(f): if len(libinfo) == len(_NEEDED_KEYS): @@ -255,7 +255,7 @@ def __repr__(self): return f"<_Lib {self}>" def __str__(self): - return "{0.name} by {0.author}, API {0.api}, patch {0.patch}".format(self) + return f"{self.name} by {self.author}, API {self.api}, patch {self.patch}" def import_module(self) -> ModuleType: if self._module is None: diff --git a/ops/main.py b/ops/main.py index 6f7b893b5..b3c013b3a 100644 --- a/ops/main.py +++ b/ops/main.py @@ -398,10 +398,7 @@ def main(charm_class: Type[ops.charm.CharmBase], metadata = (charm_dir / 'metadata.yaml').read_text() actions_meta = charm_dir / 'actions.yaml' - if actions_meta.exists(): - actions_metadata = actions_meta.read_text() - else: - actions_metadata = None + actions_metadata = actions_meta.read_text() if actions_meta.exists() else None # If we are in a RelationBroken event, we want to know which relation is # broken within the model, not only in the event's `.relation` attribute. diff --git a/ops/model.py b/ops/model.py index ad9ed81d3..4fdc39deb 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1133,13 +1133,13 @@ def from_dict(cls, id: str, d: Dict[str, Any]) -> 'SecretInfo': def __repr__(self): return ('SecretInfo(' - 'id={self.id!r}, ' - 'label={self.label!r}, ' - 'revision={self.revision}, ' - 'expires={self.expires!r}, ' - 'rotation={self.rotation}, ' - 'rotates={self.rotates!r})' - ).format(self=self) + f'id={self.id!r}, ' + f'label={self.label!r}, ' + f'revision={self.revision}, ' + f'expires={self.expires!r}, ' + f'rotation={self.rotation}, ' + f'rotates={self.rotates!r})' + ) class Secret: @@ -1780,7 +1780,7 @@ def from_name(cls, name: str, message: str): @classmethod def register(cls, child: Type['StatusBase']): """Register a Status for the child's name.""" - if not isinstance(getattr(child, 'name'), str): + if not isinstance(child.name, str): raise TypeError(f"Can't register StatusBase subclass {child}: ", "missing required `name: str` class attribute") cls._statuses[child.name] = child @@ -1949,7 +1949,7 @@ def __iter__(self): def __getitem__(self, storage_name: str) -> List['Storage']: if storage_name not in self._storage_map: - meant = ', or '.join(repr(k) for k in self._storage_map.keys()) + meant = ', or '.join(repr(k) for k in self._storage_map) raise KeyError( f'Storage {storage_name!r} not found. Did you mean {meant}?') storage_list = self._storage_map[storage_name] @@ -1970,8 +1970,8 @@ def request(self, storage_name: str, count: int = 1): ModelError: if the storage is not in the charm's metadata. """ if storage_name not in self._storage_map: - raise ModelError(('cannot add storage {!r}:' - ' it is not present in the charm metadata').format(storage_name)) + raise ModelError(f'cannot add storage {storage_name!r}:' + ' it is not present in the charm metadata') self._backend.storage_add(storage_name, count) def _invalidate(self, storage_name: str): @@ -3004,7 +3004,11 @@ def __init__(self, unit_name: Optional[str] = None, def _run(self, *args: str, return_output: bool = False, use_json: bool = False, input_stream: Optional[str] = None ) -> Union[str, Any, None]: - kwargs = dict(stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True, encoding='utf-8') + kwargs = { + 'stdout': subprocess.PIPE, + 'stderr': subprocess.PIPE, + 'check': True, + 'encoding': 'utf-8'} if input_stream: kwargs.update({"input": input_stream}) which_cmd = shutil.which(args[0]) @@ -3534,12 +3538,12 @@ def validate_metric_label(cls, label_name: str): @classmethod def format_metric_value(cls, value: Union[int, float]): if not isinstance(value, (int, float)): # pyright: ignore[reportUnnecessaryIsInstance] - raise ModelError('invalid metric value {!r} provided:' - ' must be a positive finite float'.format(value)) + raise ModelError(f'invalid metric value {value!r} provided:' + ' must be a positive finite float') if math.isnan(value) or math.isinf(value) or value < 0: - raise ModelError('invalid metric value {!r} provided:' - ' must be a positive finite float'.format(value)) + raise ModelError(f'invalid metric value {value!r} provided:' + ' must be a positive finite float') return str(value) @classmethod diff --git a/ops/pebble.py b/ops/pebble.py index 72763407a..cc091de91 100644 --- a/ops/pebble.py +++ b/ops/pebble.py @@ -561,13 +561,13 @@ def from_dict(cls, d: '_WarningDict') -> 'Warning': def __repr__(self): return ('Warning(' - 'message={self.message!r}, ' - 'first_added={self.first_added!r}, ' - 'last_added={self.last_added!r}, ' - 'last_shown={self.last_shown!r}, ' - 'expire_after={self.expire_after!r}, ' - 'repeat_after={self.repeat_after!r})' - ).format(self=self) + f'message={self.message!r}, ' + f'first_added={self.first_added!r}, ' + f'last_added={self.last_added!r}, ' + f'last_shown={self.last_shown!r}, ' + f'expire_after={self.expire_after!r}, ' + f'repeat_after={self.repeat_after!r})' + ) class TaskProgress: @@ -594,10 +594,10 @@ def from_dict(cls, d: '_ProgressDict') -> 'TaskProgress': def __repr__(self): return ('TaskProgress(' - 'label={self.label!r}, ' - 'done={self.done!r}, ' - 'total={self.total!r})' - ).format(self=self) + f'label={self.label!r}, ' + f'done={self.done!r}, ' + f'total={self.total!r})' + ) class TaskID(str): @@ -650,16 +650,16 @@ def from_dict(cls, d: '_TaskDict') -> 'Task': def __repr__(self): return ('Task(' - 'id={self.id!r}, ' - 'kind={self.kind!r}, ' - 'summary={self.summary!r}, ' - 'status={self.status!r}, ' - 'log={self.log!r}, ' - 'progress={self.progress!r}, ' - 'spawn_time={self.spawn_time!r}, ' - 'ready_time={self.ready_time!r}, ' - 'data={self.data!r})' - ).format(self=self) + f'id={self.id!r}, ' + f'kind={self.kind!r}, ' + f'summary={self.summary!r}, ' + f'status={self.status!r}, ' + f'log={self.log!r}, ' + f'progress={self.progress!r}, ' + f'spawn_time={self.spawn_time!r}, ' + f'ready_time={self.ready_time!r}, ' + f'data={self.data!r})' + ) class ChangeID(str): @@ -715,17 +715,17 @@ def from_dict(cls, d: '_ChangeDict') -> 'Change': def __repr__(self): return ('Change(' - 'id={self.id!r}, ' - 'kind={self.kind!r}, ' - 'summary={self.summary!r}, ' - 'status={self.status!r}, ' - 'tasks={self.tasks!r}, ' - 'ready={self.ready!r}, ' - 'err={self.err!r}, ' - 'spawn_time={self.spawn_time!r}, ' - 'ready_time={self.ready_time!r}, ' - 'data={self.data!r})' - ).format(self=self) + f'id={self.id!r}, ' + f'kind={self.kind!r}, ' + f'summary={self.summary!r}, ' + f'status={self.status!r}, ' + f'tasks={self.tasks!r}, ' + f'ready={self.ready!r}, ' + f'err={self.err!r}, ' + f'spawn_time={self.spawn_time!r}, ' + f'ready_time={self.ready_time!r}, ' + f'data={self.data!r})' + ) class Plan: @@ -808,7 +808,7 @@ class Layer: log_targets: Dict[str, 'LogTarget'] def __init__(self, raw: Optional[Union[str, 'LayerDict']] = None): - if isinstance(raw, str): + if isinstance(raw, str): # noqa: SIM108 d = yaml.safe_load(raw) or {} # type: ignore # (Any 'raw' type) else: d = raw or {} @@ -990,10 +990,10 @@ def from_dict(cls, d: '_ServiceInfoDict') -> 'ServiceInfo': def __repr__(self): return ('ServiceInfo(' - 'name={self.name!r}, ' - 'startup={self.startup}, ' - 'current={self.current})' - ).format(self=self) + f'name={self.name!r}, ' + f'startup={self.startup}, ' + f'current={self.current})' + ) class Check: @@ -1201,17 +1201,17 @@ def from_dict(cls, d: '_FileInfoDict') -> 'FileInfo': def __repr__(self): return ('FileInfo(' - 'path={self.path!r}, ' - 'name={self.name!r}, ' - 'type={self.type}, ' - 'size={self.size}, ' - 'permissions=0o{self.permissions:o}, ' - 'last_modified={self.last_modified!r}, ' - 'user_id={self.user_id}, ' - 'user={self.user!r}, ' - 'group_id={self.group_id}, ' - 'group={self.group!r})' - ).format(self=self) + f'path={self.path!r}, ' + f'name={self.name!r}, ' + f'type={self.type}, ' + f'size={self.size}, ' + f'permissions=0o{self.permissions:o}, ' + f'last_modified={self.last_modified!r}, ' + f'user_id={self.user_id}, ' + f'user={self.user!r}, ' + f'group_id={self.group_id}, ' + f'group={self.group!r})' + ) class CheckInfo: @@ -1284,12 +1284,12 @@ def from_dict(cls, d: '_CheckInfoDict') -> 'CheckInfo': def __repr__(self): return ('CheckInfo(' - 'name={self.name!r}, ' - 'level={self.level!r}, ' - 'status={self.status}, ' - 'failures={self.failures}, ' - 'threshold={self.threshold!r})' - ).format(self=self) + f'name={self.name!r}, ' + f'level={self.level!r}, ' + f'status={self.status}, ' + f'failures={self.failures}, ' + f'threshold={self.threshold!r})' + ) class NoticeType(enum.Enum): @@ -1783,7 +1783,7 @@ def _request_raw( if headers is None: headers = {} - request = urllib.request.Request(url, method=method, data=data, headers=headers) + request = urllib.request.Request(url, method=method, data=data, headers=headers) # noqa: S310 try: response = self.opener.open(request, timeout=self.timeout) @@ -2027,9 +2027,11 @@ def _wait_change(self, change_id: ChangeID, timeout: Optional[float] = None) -> resp = self._request('GET', f'/v1/changes/{change_id}/wait', query) except APIError as e: if e.code == 404: - raise NotImplementedError('server does not implement wait-change endpoint') + raise NotImplementedError( + 'server does not implement wait-change endpoint') from None if e.code == 504: - raise TimeoutError(f'timed out waiting for change {change_id} ({timeout} seconds)') + raise TimeoutError( + f'timed out waiting for change {change_id} ({timeout} seconds)') from None raise return Change.from_dict(resp['result']) @@ -2257,8 +2259,8 @@ def _encode_multipart(self, metadata: Dict[str, Any], path: str, else: source_io: _AnyStrFileLikeIO = source # type: ignore boundary = binascii.hexlify(os.urandom(16)) - path_escaped = path.replace('"', '\\"').encode('utf-8') # NOQA: test_quote_backslashes - content_type = f"multipart/form-data; boundary=\"{boundary.decode('utf-8')}\"" # NOQA: test_quote_backslashes + path_escaped = path.replace('"', '\\"').encode('utf-8') + content_type = f"multipart/form-data; boundary=\"{boundary.decode('utf-8')}\"" def generator() -> Generator[bytes, None, None]: yield b''.join([ @@ -2612,8 +2614,8 @@ def exec( # finishing early with an error. Call wait_change to pick that up. change = self.wait_change(ChangeID(change_id)) if change.err: - raise ChangeError(change.err, change) - raise ConnectionError(f'unexpected error connecting to websockets: {e}') + raise ChangeError(change.err, change) from e + raise ConnectionError(f'unexpected error connecting to websockets: {e}') from e cancel_stdin: Optional[Callable[[], None]] = None cancel_reader: Optional[int] = None @@ -2707,7 +2709,7 @@ def send_signal(self, sig: Union[int, str], services: Iterable[str]): """ if isinstance(services, (str, bytes)) or not hasattr(services, '__iter__'): raise TypeError('services must be of type Iterable[str], ' - 'not {}'.format(type(services).__name__)) + f'not {type(services).__name__}') for s in services: if not isinstance(s, str): raise TypeError(f'service names must be str, not {type(s).__name__}') @@ -2928,7 +2930,7 @@ def get_file(self, path: str, encoding: Optional[str]) -> '_TextOrBinaryIO': # We're using text-based file I/O purely for file encoding purposes, not for # newline normalization. newline='' serves the line endings as-is. newline = '' if encoding else None - file_io = open(self._files[path].name, mode, + file_io = open(self._files[path].name, mode, # noqa: SIM115 encoding=encoding, newline=newline) # open() returns IO[Any] return typing.cast('_TextOrBinaryIO', file_io) diff --git a/ops/storage.py b/ops/storage.py index f8a0df816..80d610807 100644 --- a/ops/storage.py +++ b/ops/storage.py @@ -142,7 +142,7 @@ def load_snapshot(self, handle_path: str) -> Any: c.execute("SELECT data FROM snapshot WHERE handle=?", (handle_path,)) row = c.fetchone() if row: - return pickle.loads(row[0]) + return pickle.loads(row[0]) # noqa: S301 raise NoSnapshotError(handle_path) def drop_snapshot(self, handle_path: str): @@ -393,7 +393,7 @@ def get(self, key: str) -> Any: p = _run(["state-get", key], stdout=subprocess.PIPE, check=True) if p.stdout == '' or p.stdout == '\n': raise KeyError(key) - return yaml.load(p.stdout, Loader=_SimpleLoader) # type: ignore + return yaml.load(p.stdout, Loader=_SimpleLoader) # type: ignore # noqa: S506 def delete(self, key: str) -> None: """Remove a key from being tracked. diff --git a/ops/testing.py b/ops/testing.py index 5d509cb3f..4eba4f96d 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -595,7 +595,7 @@ def add_oci_resource(self, resource_name: str, 'username': 'username', 'password': 'password', } - if resource_name not in self._meta.resources.keys(): + if resource_name not in self._meta.resources: raise RuntimeError(f'Resource {resource_name} is not a defined resources') if self._meta.resources[resource_name].type != "oci-image": raise RuntimeError(f'Resource {resource_name} is not an OCI Image') @@ -614,7 +614,7 @@ def add_resource(self, resource_name: str, content: AnyStr) -> None: content: Either string or bytes content, which will be the content of the filename returned by resource-get. If contents is a string, it will be encoded in utf-8 """ - if resource_name not in self._meta.resources.keys(): + if resource_name not in self._meta.resources: raise RuntimeError(f'Resource {resource_name} is not a defined resource') record = self._meta.resources[resource_name] if record.type != "file": @@ -1706,12 +1706,9 @@ def test_hostname(self): Return: The path of the temporary directory associated with the specified container. """ - # it's okay to access the container directly in this context, as its creation has already + # It's okay to access the container directly in this context, as its creation has already # been ensured during the model's initialization. - if isinstance(container, str): - container_name = container - else: - container_name = container.name + container_name = container if isinstance(container, str) else container.name return self._backend._pebble_clients[container_name]._root def evaluate_status(self) -> None: @@ -1866,7 +1863,7 @@ def run_action(self, action_name: str, try: action_meta = self.charm.meta.actions[action_name] except KeyError: - raise RuntimeError(f"Charm does not have a {action_name!r} action.") + raise RuntimeError(f"Charm does not have a {action_name!r} action.") from None if params is None: params = {} for key in action_meta.required: @@ -1896,9 +1893,7 @@ def run_action(self, action_name: str, def _get_app_or_unit_name(app_or_unit: AppUnitOrName) -> str: """Return name of given application or unit (return strings directly).""" - if isinstance(app_or_unit, model.Application): - return app_or_unit.name - elif isinstance(app_or_unit, model.Unit): + if isinstance(app_or_unit, (model.Application, model.Unit)): return app_or_unit.name elif isinstance(app_or_unit, str): return app_or_unit @@ -1918,9 +1913,9 @@ def _record_calls(cls: Any): def decorator(orig_method: Any): def wrapped(self: '_TestingModelBackend', *args: Any, **kwargs: Any): - full_args = (orig_method.__name__,) + args + full_args = (orig_method.__name__, *args) if kwargs: - full_args = full_args + (kwargs,) + full_args = (*full_args, kwargs) self._calls.append(full_args) return orig_method(self, *args, **kwargs) return wrapped @@ -1940,7 +1935,7 @@ class TargetClass: __doc__ for that method. """ def decorator(target_cls: Any): - for meth_name, _ in target_cls.__dict__.items(): + for meth_name in target_cls.__dict__: if meth_name.startswith('_'): continue source_method = source_cls.__dict__.get(meth_name) @@ -1987,15 +1982,15 @@ def _config_set(self, key: str, value: Union[str, int, float, bool]): # has the expected type. option = self._spec.get('options', {}).get(key) if not option: - raise RuntimeError('Unknown config option {}; ' + raise RuntimeError(f'Unknown config option {key}; ' 'not declared in `config.yaml`.' 'Check https://juju.is/docs/sdk/config for the ' - 'spec.'.format(key)) + 'spec.') declared_type = option.get('type') if not declared_type: - raise RuntimeError('Incorrectly formatted `options.yaml`, option {} ' - 'is expected to declare a `type`.'.format(key)) + raise RuntimeError(f'Incorrectly formatted `options.yaml`, option {key} ' + 'is expected to declare a `type`.') if declared_type not in self._supported_types: raise RuntimeError( @@ -2003,9 +1998,8 @@ def _config_set(self, key: str, value: Union[str, int, float, bool]): 'of [{}], not {}.'.format(', '.join(self._supported_types), declared_type)) if type(value) is not self._supported_types[declared_type]: - raise RuntimeError('Config option {} is supposed to be of type ' - '{}, not `{}`.'.format(key, declared_type, - type(value).__name__)) + raise RuntimeError(f'Config option {key} is supposed to be of type ' + f'{declared_type}, not `{type(value).__name__}`.') # call 'normal' setattr. dict.__setitem__(self, key, value) # type: ignore @@ -2126,8 +2120,7 @@ def _validate_relation_access(self, relation_name: str, relations: List[model.Re if self._hook_is_running == 'leader_elected' and relation_name in valid_relation_endpoints: raise RuntimeError( 'cannot access relation data without first adding the relation: ' - 'use Harness.add_relation({!r}, ) before calling set_leader' - .format(relation_name)) + f'use Harness.add_relation({relation_name!r}, ) before calling set_leader') def _can_connect(self, pebble_client: '_TestingPebbleClient') -> bool: """Returns whether the mock client is active and can support API calls with no errors.""" @@ -2217,10 +2210,7 @@ def relation_set(self, relation_id: int, key: str, value: str, is_app: bool): raise RelationNotFoundError(relation_id) relation = self._relation_data_raw[relation_id] - if is_app: - bucket_key = self.app_name - else: - bucket_key = self.unit_name + bucket_key = self.app_name if is_app else self.unit_name if bucket_key not in relation: relation[bucket_key] = {} bucket = relation[bucket_key] @@ -2249,10 +2239,7 @@ def resource_get(self, resource_name: str): resource_dir = self._get_resource_dir() resource_filename = resource_dir / resource_name / filename if not resource_filename.exists(): - if isinstance(contents, bytes): - mode = 'wb' - else: - mode = 'wt' + mode = 'wb' if isinstance(contents, bytes) else 'wt' resource_filename.parent.mkdir(exist_ok=True) with resource_filename.open(mode=mode) as resource_file: resource_file.write(contents) @@ -2283,8 +2270,8 @@ def storage_list(self, name: str, include_detached: bool = False): name: name (i.e. from metadata.yaml). include_detached: True to include unattached storage mounts as well. """ - return list(index for index in self._storage_list[name] - if include_detached or self._storage_is_attached(name, index)) + return [index for index in self._storage_list[name] + if include_detached or self._storage_is_attached(name, index)] def storage_get(self, storage_name_id: str, attribute: str) -> Any: name, index = storage_name_id.split("/", 1) @@ -2321,7 +2308,7 @@ def _storage_detach(self, storage_id: str): index = int(index) for container, client in self._pebble_clients.items(): - for _, mount in self._meta.containers[container].mounts.items(): + for mount in self._meta.containers[container].mounts.values(): if mount.storage != name: continue root = client._root @@ -2338,10 +2325,10 @@ def _storage_attach(self, storage_id: str): name, index = storage_id.split('/', 1) for container, client in self._pebble_clients.items(): - for _, mount in self._meta.containers[container].mounts.items(): + for mount in self._meta.containers[container].mounts.values(): if mount.storage != name: continue - for index, store in self._storage_list[mount.storage].items(): + for store in self._storage_list[mount.storage].values(): root = client._root mounting_dir = root / mount.location[1:] mounting_dir.parent.mkdir(parents=True, exist_ok=True) @@ -2371,9 +2358,9 @@ def action_get(self) -> Dict[str, Any]: params: Dict[str, Any] = {} assert self._running_action is not None action_meta = self._meta.actions[self._running_action.name] - for name, action_meta in action_meta.parameters.items(): - if "default" in action_meta: - params[name] = action_meta["default"] + for name, meta in action_meta.parameters.items(): + if "default" in meta: + params[name] = meta["default"] params.update(self._running_action.parameters) return params @@ -2597,7 +2584,7 @@ def secret_set(self, id: str, *, @classmethod def _generate_secret_id(cls) -> str: # Not a proper Juju secrets-style xid, but that's okay - return f"secret:{str(uuid.uuid4())}" + return f"secret:{uuid.uuid4()}" def secret_add(self, content: Dict[str, str], *, label: Optional[str] = None, @@ -2605,10 +2592,7 @@ def secret_add(self, content: Dict[str, str], *, expire: Optional[datetime.datetime] = None, rotate: Optional[model.SecretRotate] = None, owner: Optional[str] = None) -> str: - if owner == 'unit': - owner_name = self.unit_name - else: - owner_name = self.app_name + owner_name = self.unit_name if owner == 'unit' else self.app_name return self._secret_add(content, owner_name, label=label, description=description, @@ -2916,11 +2900,11 @@ def add_layer( # 'override' is actually single quoted in the real error, but # it shouldn't be, hopefully that gets cleaned up. if not service.override: - raise RuntimeError('500 Internal Server Error: layer "{}" must define' - '"override" for service "{}"'.format(label, name)) + raise RuntimeError(f'500 Internal Server Error: layer "{label}" must define' + f'"override" for service "{name}"') if service.override not in ('merge', 'replace'): - raise RuntimeError('500 Internal Server Error: layer "{}" has invalid ' - '"override" value on service "{}"'.format(label, name)) + raise RuntimeError(f'500 Internal Server Error: layer "{label}" has invalid ' + f'"override" value on service "{name}"') elif service.override == 'replace': layer.services[name] = service elif service.override == 'merge': @@ -3011,9 +2995,11 @@ def pull(self, path: str, *, Union[BinaryIO, TextIO], file_path.open("rb" if encoding is None else "r", encoding=encoding)) except FileNotFoundError: - raise pebble.PathError('not-found', f'stat {path}: no such file or directory') + raise pebble.PathError('not-found', + f'stat {path}: no such file or directory') from None except IsADirectoryError: - raise pebble.PathError('generic-file-error', f'can only read a regular file: "{path}"') + raise pebble.PathError('generic-file-error', + f'can only read a regular file: "{path}"') from None def push( self, path: str, source: 'ReadableBuffer', *, @@ -3055,10 +3041,10 @@ def push( os.chmod(file_path, permissions) except FileNotFoundError as e: raise pebble.PathError( - 'not-found', f'parent directory not found: {e.args[0]}') + 'not-found', f'parent directory not found: {e.args[0]}') from None except NotADirectoryError: raise pebble.PathError('generic-file-error', - f'open {path}.~: not a directory') + f'open {path}.~: not a directory') from None def list_files(self, path: str, *, pattern: Optional[str] = None, itself: bool = False) -> List[pebble.FileInfo]: @@ -3123,10 +3109,12 @@ def make_dir( os.chmod(dir_path, permissions) except FileExistsError: if not make_parents: - raise pebble.PathError('generic-file-error', f'mkdir {path}: file exists') + raise pebble.PathError( + 'generic-file-error', + f'mkdir {path}: file exists') from None except NotADirectoryError as e: # Attempted to create a subdirectory of a file - raise pebble.PathError('generic-file-error', f'not a directory: {e.args[0]}') + raise pebble.PathError('generic-file-error', f'not a directory: {e.args[0]}') from None def remove_path(self, path: str, *, recursive: bool = False): self._check_connection() @@ -3301,7 +3289,7 @@ def send_signal(self, sig: Union[int, str], service_names: Iterable[str]): # conform with the real pebble api first_service = next(iter(service_names)) message = f'cannot send signal to "{first_service}": invalid signal name "{sig}"' - raise self._api_error(500, message) + raise self._api_error(500, message) from None def get_checks(self, level=None, names=None): # type:ignore raise NotImplementedError(self.get_checks) # type:ignore diff --git a/pyproject.toml b/pyproject.toml index 2ccda119a..e0083f020 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -82,22 +82,94 @@ select = [ "A", # flake8-copyright "CPY", + # pyupgrade + "UP", + # flake8-2020 + "YTT", + # flake8-bandit + "S", + # flake8-bugbear + "B", + # flake8-simplify + "SIM", + # Ruff specific + "RUF", + # Perflint + "PERF", # pyflakes-docstrings "D", ] ignore = [ + # Use of `assert` detected + "S101", + # Do not `assert False` + "B011", + # `pickle`, `cPickle`, `dill`, and `shelve` modules are possibly insecure + "S403", + # `subprocess` module is possibly insecure + "S404", + + # No explicit `stacklevel` keyword argument found + "B028", + + # Use contextlib.suppress() instead of try/except: pass + "SIM105", + # Use a single `with` statement with multiple contexts instead of nested `with` statements + "SIM117", + # Missing docstring in magic method "D105", # Missing docstring in `__init__` "D107", + + # Manual list comprehension. + "PERF401", + # Manual dict comprehension. + "PERF403", + + # Convert {} from `TypedDict` functional to class syntax + # Note that since we have some `TypedDict`s that cannot use the class + # syntax, we're currently choosing to be consistent in syntax even though + # some can be moved to the class syntax. + "UP013", + + ## Likely worth doing, but later. + + # `subprocess` call: check for execution of untrusted input + "S603", + # Prefer `next(iter(info_dicts))` over single element slice + "RUF015", + # Mutable class attributes should be annotated with `typing.ClassVar` + "RUF012", + # Unnecessary dict comprehension for iterable; use `dict.fromkeys` instead + "RUF025", + # Unnecessary `tuple` call (rewrite as a literal) + "C408", +] +[tool.ruff.lint.per-file-ignores] +"test/*" = [ + # All documentation linting. + "D", + + # Hard-coded password string. + "S105", + # Hard-coded password function argument. + "S106", + + # "Useless" expression. + "B018" ] -[tool.ruff.per-file-ignores] -"test/*" = ["D"] "docs/conf.py" = [ "CPY001", # Missing copyright notice at top of file "D100", # Missing docstring in public module "I001", # [*] Import block is un-sorted or un-formatted "A001", # Variable `copyright` is shadowing a Python builtin + "RUF003", # Comment contains ambiguous {} + "RUF019", # [*] Unnecessary key check before dictionary access +] +"ops/_private/timeconv.py" = [ + "RUF001", # String contains ambiguous `µ` (MICRO SIGN). Did you mean `μ` (GREEK SMALL LETTER MU)? + "RUF002", # Docstring contains ambiguous `µ` (MICRO SIGN). Did you mean `μ` (GREEK SMALL LETTER MU)? ] [tool.ruff.lint.pydocstyle] diff --git a/test/charms/test_main/src/charm.py b/test/charms/test_main/src/charm.py index fb48d5ecb..ae845c1e6 100755 --- a/test/charms/test_main/src/charm.py +++ b/test/charms/test_main/src/charm.py @@ -20,7 +20,7 @@ sys.path.append('lib') -import ops # noqa: E402 (module-level import after non-import code) +import ops logger = logging.getLogger() diff --git a/test/pebble_cli.py b/test/pebble_cli.py index 2af4128eb..573c842aa 100644 --- a/test/pebble_cli.py +++ b/test/pebble_cli.py @@ -184,15 +184,9 @@ def main(): stderr = sys.stderr.buffer if not args.combine_stderr else None else: if sys.stdin.isatty(): - if encoding is not None: - stdin = sys.stdin - else: - stdin = sys.stdin.buffer + stdin = sys.stdin.buffer if encoding is None else sys.stdin else: - if encoding is not None: - stdin = sys.stdin.read() - else: - stdin = sys.stdin.buffer.read() + stdin = sys.stdin.buffer.read() if encoding is None else sys.stdin.read() stdout = None stderr = None diff --git a/test/test_helpers.py b/test/test_helpers.py index ada79af09..09adc9c6a 100644 --- a/test/test_helpers.py +++ b/test/test_helpers.py @@ -52,7 +52,7 @@ def cleanup(): '''#!/bin/sh {{ printf {name}; printf "\\036%s" "$@"; printf "\\034"; }} >> {path}/calls.txt {content}'''.format_map(template_args)) - os.chmod(str(path), 0o755) # type: ignore + os.chmod(str(path), 0o755) # type: ignore # noqa: S103 # TODO: this hardcodes the path to bash.exe, which works for now but might # need to be set via environ or something like that. path.with_suffix(".bat").write_text( # type: ignore diff --git a/test/test_infra.py b/test/test_infra.py index 33cf2e9a3..3ea47fb95 100644 --- a/test/test_infra.py +++ b/test/test_infra.py @@ -56,7 +56,7 @@ def check(self, name: str): fd, testfile = tempfile.mkstemp() self.addCleanup(os.unlink, testfile) - with open(fd, 'wt', encoding='utf8') as fh: + with open(fd, 'w', encoding='utf8') as fh: fh.write(self.template.format(module_name=name)) environ = os.environ.copy() diff --git a/test/test_lib.py b/test/test_lib.py index f19aa2ec1..1a68bc30b 100644 --- a/test/test_lib.py +++ b/test/test_lib.py @@ -327,7 +327,7 @@ def test_other_encoding(self): if m.origin is None: self.assertIsNotNone(m.origin) return - with open(m.origin, 'wt', encoding='latin-1') as f: + with open(m.origin, 'w', encoding='latin-1') as f: f.write(dedent(''' LIBNAME = "foo" LIBAPI = 2 @@ -362,9 +362,9 @@ def test_lib_comparison(self): ops.lib._Lib(_dummy_spec, "foo", "alice@example.com", 1, 1)) with self.assertRaises(TypeError): - 42 < ops.lib._Lib(_dummy_spec, "bar", "alice@example.com", 1, 1) # type:ignore + 42 < ops.lib._Lib(_dummy_spec, "bar", "alice@example.com", 1, 1) # type:ignore # noqa: B015, SIM300 with self.assertRaises(TypeError): - ops.lib._Lib(_dummy_spec, "bar", "alice@example.com", 1, 1) < 42 # type: ignore + ops.lib._Lib(_dummy_spec, "bar", "alice@example.com", 1, 1) < 42 # type: ignore # noqa: B015 # these two might be surprising in that they don't raise an exception, # but they are correct: our __eq__ bailing means Python falls back to diff --git a/test/test_main.py b/test/test_main.py index 3c9d5ecef..83f541bf6 100644 --- a/test/test_main.py +++ b/test/test_main.py @@ -949,7 +949,7 @@ def test_setup_event_links(self): Symlink creation caused by initial events should _not_ happen when using dispatch. """ all_event_hooks = [f"hooks/{e.replace('_', '-')}" - for e in self.charm_module.Charm.on.events().keys()] + for e in self.charm_module.Charm.on.events()] initial_events = { EventSpec(ops.InstallEvent, 'install'), EventSpec(ops.StorageAttachedEvent, 'disks-storage-attached'), diff --git a/test/test_pebble.py b/test/test_pebble.py index b8eb6a442..4709e94f0 100644 --- a/test/test_pebble.py +++ b/test/test_pebble.py @@ -1404,14 +1404,17 @@ def test_multipart_parser(self): bodies: typing.List[bytes] = [] bodies_done: typing.List[bool] = [] + # All of the "noqa: B023" here are due to a ruff bug: + # https://github.com/astral-sh/ruff/issues/7847 + # ruff should tell us when the 'noqa's are no longer required. def handle_header(data: typing.Any): - headers.append(bytes(data)) - bodies.append(b'') - bodies_done.append(False) + headers.append(bytes(data)) # noqa: B023 + bodies.append(b'') # noqa: B023 + bodies_done.append(False) # noqa: B023 def handle_body(data: bytes, done: bool = False): - bodies[-1] += data - bodies_done[-1] = done + bodies[-1] += data # noqa: B023 + bodies_done[-1] = done # noqa: B023 parser = pebble._MultipartParser( marker, diff --git a/test/test_private.py b/test/test_private.py index e415d19da..45ef60307 100644 --- a/test/test_private.py +++ b/test/test_private.py @@ -127,7 +127,7 @@ def test_parse_duration(self): # different units ('10ns', datetime.timedelta(seconds=0.000_000_010)), ('11us', datetime.timedelta(seconds=0.000_011)), - ('12µs', datetime.timedelta(seconds=0.000_012)), # U+00B5 + ('12µs', datetime.timedelta(seconds=0.000_012)), # U+00B5 # noqa: RUF001 ('12μs', datetime.timedelta(seconds=0.000_012)), # U+03BC ('13ms', datetime.timedelta(seconds=0.013)), ('14s', datetime.timedelta(seconds=14)), diff --git a/test/test_real_pebble.py b/test/test_real_pebble.py index 99f2ab10d..d5074788e 100644 --- a/test/test_real_pebble.py +++ b/test/test_real_pebble.py @@ -234,9 +234,7 @@ def stdin_thread(): threading.Thread(target=stdin_thread).start() - reads: typing.List[str] = [] - for line in process.stdout: - reads.append(line) + reads: typing.List[str] = list(process.stdout) process.wait() @@ -258,9 +256,7 @@ def stdin_thread(): threading.Thread(target=stdin_thread).start() - reads: typing.List[bytes] = [] - for line in process.stdout: - reads.append(line) + reads: typing.List[bytes] = list(process.stdout) process.wait() diff --git a/test/test_storage.py b/test/test_storage.py index b8467c8bb..517526676 100644 --- a/test/test_storage.py +++ b/test/test_storage.py @@ -352,7 +352,7 @@ def test_is_c_dumper(self): def test_handles_tuples(self): raw = yaml.dump((1, 'tuple'), Dumper=ops.storage._SimpleDumper) - parsed = yaml.load(raw, Loader=ops.storage._SimpleLoader) + parsed = yaml.load(raw, Loader=ops.storage._SimpleLoader) # noqa: S506 self.assertEqual(parsed, (1, 'tuple')) def assertRefused(self, obj: typing.Any): # noqa: N802 @@ -362,7 +362,7 @@ def assertRefused(self, obj: typing.Any): # noqa: N802 # If they did somehow end up written, we shouldn't be able to load them raw = yaml.dump(obj, Dumper=yaml.Dumper) with self.assertRaises(yaml.constructor.ConstructorError): - yaml.load(raw, Loader=ops.storage._SimpleLoader) + yaml.load(raw, Loader=ops.storage._SimpleLoader) # noqa: S506 def test_forbids_some_types(self): self.assertRefused(1 + 2j) @@ -444,7 +444,7 @@ def test_set_and_get_complex_value(self): outer = yaml.safe_load(content) key = 'Class[foo]/_stored' self.assertEqual(list(outer.keys()), [key]) - inner = yaml.load(outer[key], Loader=ops.storage._SimpleLoader) + inner = yaml.load(outer[key], Loader=ops.storage._SimpleLoader) # noqa: S506 self.assertEqual(complex_val, inner) self.assertEqual(content.decode('utf-8'), dedent("""\ "Class[foo]/_stored": | diff --git a/test/test_testing.py b/test/test_testing.py index a2d0cfd3b..0e5db2214 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -2176,8 +2176,8 @@ def test_container_isdir_and_exists(self): harness.set_can_connect('foo', True) c = harness.model.unit.containers['foo'] - dir_path = '/tmp/foo/dir' - file_path = '/tmp/foo/file' + dir_path = '/tmp/foo/dir' # noqa: S108 + file_path = '/tmp/foo/file' # noqa: S108 self.assertFalse(c.isdir(dir_path)) self.assertFalse(c.exists(dir_path)) @@ -4704,7 +4704,7 @@ def test_push_path(self): (tempdir / "foo/test").write_text("test") (tempdir / "foo/bar/foobar").write_text("foobar") (tempdir / "foo/baz").mkdir(parents=True) - self.container.push_path(tempdir / "foo", "/tmp") + self.container.push_path(tempdir / "foo", "/tmp") # noqa: S108 self.assertTrue((self.root / "tmp").is_dir()) self.assertTrue((self.root / "tmp/foo").is_dir()) @@ -4714,7 +4714,7 @@ def test_push_path(self): self.assertEqual((self.root / "tmp/foo/bar/foobar").read_text(), "foobar") def test_make_dir(self): - self.container.make_dir("/tmp") + self.container.make_dir("/tmp") # noqa: S108 self.assertTrue((self.root / "tmp").is_dir()) self.container.make_dir("/foo/bar/foobar", make_parents=True) self.assertTrue((self.root / "foo/bar/foobar").is_dir()) @@ -5382,7 +5382,7 @@ def test_exec_stdout_stderr(self): def test_exec_service_context(self): service: ops.pebble.ServiceDict = { "command": "test", - "working-dir": "/tmp", + "working-dir": "/tmp", # noqa: S108 "user": "foo", "user-id": 1, "group": "bar", @@ -5404,7 +5404,7 @@ def handler(args: ops.testing.ExecArgs): self.harness.handle_exec(self.container, ["ls"], handler=handler) self.container.exec(["ls"], service_context="test").wait() - self.assertEqual(args_history[-1].working_dir, "/tmp") + self.assertEqual(args_history[-1].working_dir, "/tmp") # noqa: S108 self.assertEqual(args_history[-1].user, "foo") self.assertEqual(args_history[-1].user_id, 1) self.assertEqual(args_history[-1].group, "bar") diff --git a/tox.ini b/tox.ini index 09b0ef4fc..d1ba3fe0f 100644 --- a/tox.ini +++ b/tox.ini @@ -44,7 +44,7 @@ commands = [testenv:lint] description = Check code against coding style standards deps = - ruff~=0.1.4 + ruff~=0.2.1 commands = ruff check --preview