Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: expand ruff rule sets and make corresponding minor adjustments #1114

Merged
merged 32 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
bd877cc
Move to ruff for linting.
tonyandrewmeyer Jan 26, 2024
f6e3b02
Fix docstring ignore.
tonyandrewmeyer Jan 26, 2024
bd89726
Fix bad re-import.
tonyandrewmeyer Jan 26, 2024
8e79ae7
Fix bad auto-fix.
tonyandrewmeyer Jan 26, 2024
2f52fa8
Fix bad auto-fix.
tonyandrewmeyer Jan 26, 2024
66879bb
Remove out-of-date test.
tonyandrewmeyer Jan 26, 2024
4203eb7
We are deliberately testing in this order.
tonyandrewmeyer Jan 26, 2024
2112b29
Adjust fix so that pyright is happy.
tonyandrewmeyer Jan 26, 2024
79110c0
Remove notes.
tonyandrewmeyer Jan 26, 2024
ad50df9
C was not properly enabled before, so we can leave it disabled now.
tonyandrewmeyer Feb 2, 2024
8d8da24
Revert unnecessary formatting change.
tonyandrewmeyer Feb 2, 2024
2f02ac4
Revert unnecessary formatting change.
tonyandrewmeyer Feb 2, 2024
debd5ba
Revert unnecessary formatting change.
tonyandrewmeyer Feb 2, 2024
c0f77ac
Revert unnecessary formatting change.
tonyandrewmeyer Feb 2, 2024
abc838b
Revert unnecessary formatting change.
tonyandrewmeyer Feb 2, 2024
c7c457b
Revert unnecessary formatting change.
tonyandrewmeyer Feb 2, 2024
e8d7999
Revert unnecessary formatting change.
tonyandrewmeyer Feb 2, 2024
e4517c3
Our loader is a subclass of safe loader, so safe.
tonyandrewmeyer Feb 2, 2024
5b678fd
Merge branch 'main' into ruff-linting-1102
tonyandrewmeyer Feb 2, 2024
1249d11
Merge branch 'main' into ruff-linting-1102
tonyandrewmeyer Feb 6, 2024
4e973c3
Merge branch 'main' into ruff-linting-1102
tonyandrewmeyer Feb 14, 2024
0ecd66e
Remove ANN and PT for now.
tonyandrewmeyer Feb 14, 2024
2a11df2
More fully remove ANN.
tonyandrewmeyer Feb 14, 2024
bea9853
Make it clearer what's being skipped for the tests.
tonyandrewmeyer Feb 14, 2024
f67d053
Improvement from Ben that the linter missed.
tonyandrewmeyer Feb 14, 2024
2139898
Don't use contextlib.suppress.
tonyandrewmeyer Feb 14, 2024
7050a22
Bump ruff version.
tonyandrewmeyer Feb 14, 2024
ada2255
Merge branch 'main' into ruff-linting-1102
tonyandrewmeyer Feb 15, 2024
b6921a0
Disable PERF401 and PERF403 globally.
tonyandrewmeyer Feb 15, 2024
cf5b2ab
Add comment.
tonyandrewmeyer Feb 15, 2024
0276163
Remove duplicate ignore.
tonyandrewmeyer Feb 15, 2024
d183021
Add comment.
tonyandrewmeyer Feb 15, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ops/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
2 changes: 1 addition & 1 deletion ops/_private/yaml.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
22 changes: 10 additions & 12 deletions ops/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 charms software is in a running state. Note that
the charms software should be configured so as to persist in this state
through reboots without further intervention on Jujus part.
used to ensure that the charm's software is in a running state. Note that
tonyandrewmeyer marked this conversation as resolved.
Show resolved Hide resolved
the charm's software should be configured so as to persist in this state
through reboots without further intervention on Juju's part.
"""


Expand All @@ -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
units destruction sequence. Callback methods bound to this event
should be used to ensure that the charms 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.
"""

Expand Down Expand Up @@ -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 relations
The settings that may be queried, or set, are determined by the relation's
interface.
"""

Expand All @@ -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 theres no
guarantee that its still part of the system; its 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.

Expand Down Expand Up @@ -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 charms 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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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):
Expand Down
8 changes: 4 additions & 4 deletions ops/lib/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from types import ModuleType
from typing import List

__all__ = ('use', 'autoimport')
__all__ = ('autoimport', 'use')

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -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):
Expand All @@ -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):
Expand Down Expand Up @@ -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:
Expand Down
5 changes: 1 addition & 4 deletions ops/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this a drive-by, or something the tool caught? I don't mind this change, but just wanting to reduce additional changes in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's something the tool caught, but could be skipped to make the PR minimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left it in for this PR where it fits more (general minor improvements caught by a few extra linters) than in the first one (strictly migrate to ruff).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I think we should disable this one, but looking over the PR I think it's almost always an improvement, so this seems reasonable to keep enabled.


# 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.
Expand Down
36 changes: 20 additions & 16 deletions ops/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]
Expand All @@ -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):
Expand Down Expand Up @@ -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])
Expand Down Expand Up @@ -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
Expand Down
Loading
Loading