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

Lint #6161

Merged
merged 4 commits into from
Jul 18, 2024
Merged

Lint #6161

merged 4 commits into from
Jul 18, 2024

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Jun 19, 2024

Small lint fixes for rules not currently covered by our CI, low priority.

Took ruff for a spin to see how handy its code fixers are (it's not just a linter, it can fix some issues for you). Some of the fixers are kinda useful, but most the good rules have to be done manually, so turns out they aren't that helpful for us, worth a try, never mind.

Kept a four of the fixes and turned them into this PR. They're mostly small, but B904 created a good bit of diff. Long story short, if you're re-raising an exception with a more helpful class/message, you generally want to do raise Exception(...) from None, otherwise the context of the exception you're re-raising is included in the traceback which kinda defates the object.

I.E:

from cylc.flow.exceptions import InputError

try:
    # the general exception we are catching
    raise Exception('foo')
except Exception as exc:
    # the specific exception we are raising
    raise InputError('bar')

Preserves the parent exception context resulting in a confusing traceback:

Traceback (most recent call last):
  File "/var/tmp/tmp.wlysihZu3M/mess.py", line 5, in <module>
    raise Exception('foo')
Exception: foo

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/var/tmp/tmp.wlysihZu3M/mess.py", line 8, in <module>
    raise InputError('bar')
cylc.flow.exceptions.InputError: bar

Whereas:

from cylc.flow.exceptions import InputError

try:
    # the general exception we are catching
    raise Exception('foo')
except Exception as exc:
    # the specific exception we are raising
    raise InputError('bar') from None

Chops off the parent context:

Traceback (most recent call last):
  File "/var/tmp/tmp.wlysihZu3M/mess.py", line 8, in <module>
    raise InputError('bar') from None
cylc.flow.exceptions.InputError: bar

Using from exc is the same as the default behaviour (just more explicit).

Raising the whole exception tree is a useful feature for debugging uncaught errors, but not so good for errors we are trying to niceify for users.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@oliver-sanders oliver-sanders added this to the 8.4.0 milestone Jun 19, 2024
@oliver-sanders oliver-sanders self-assigned this Jun 19, 2024
@@ -72,7 +72,7 @@ def get_broadcast_change_iter(modified_settings, is_cancel=False):
value = setting
keys_str = ""
while isinstance(value, dict):
key, value = list(value.items())[0]
key, value = next(iter(value.items()))
Copy link
Member Author

@oliver-sanders oliver-sanders Jul 12, 2024

Choose a reason for hiding this comment

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

RUF015: Get the first item from the iterator rather than working through the whole iterator, turning it into a list, then getting the first item.

@@ -129,7 +129,7 @@ def _clean_check(opts: 'Values', id_: str, run_dir: Path) -> None:
except ContactFileExists as exc:
raise ServiceFileError(
f"Cannot clean running workflow {id_}.\n\n{exc}"
)
) from None
Copy link
Member Author

@oliver-sanders oliver-sanders Jul 12, 2024

Choose a reason for hiding this comment

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

B904: Don't include the ContactFileExists exception context in the ServiceFileError.

@@ -235,7 +235,7 @@ def _report(
"""
try:
ret: List[Tuple[Optional[str], Optional[str], bool]] = []
for _mutation_name, mutation_response in response.items():
for mutation_response in response.values():
Copy link
Member Author

Choose a reason for hiding this comment

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

PERF101: don't needless iterate items

keys.append(api_key)
shuffle(keys)
return keys[0].strip()
return choice(list(api_keys)).strip() # nosec
Copy link
Member Author

Choose a reason for hiding this comment

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

PERF402: Don't build an entire list when you only need one item

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Nice, worth doing!

@hjoliver hjoliver merged commit c24ff05 into cylc:master Jul 18, 2024
26 of 27 checks passed
@oliver-sanders oliver-sanders deleted the lint-- branch July 18, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants