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

@@ -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',

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:

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

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:

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.

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