-
Notifications
You must be signed in to change notification settings - Fork 21
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
Enforce linting rule RUF012 #259
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment below is mostly aesthetic. I don't think it's harmful as-is.
e.__class__.__name__, | ||
str(e), | ||
) | ||
msg = f"Loading task '{task_path}' caused error {e.__class__.__name__}:\n\t{e!s}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for embedding e
in the re-raised CaputConfigError
here (I'm mostly talking about the \t{e!s}
part)?
raise BarError from FooError
is already going to report the original FooError
in one of its two tracebacks.
And if something else is catching the CaputConfigError
, isn't it better for that to get the original e
out of CaputConfigError.__cause__
if it wants it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly? I'm not sure. I think it was just written this way when the original pipeline runner was put together years and years ago.
I have another branch where I'm making improvements to the pipeline runner, so I'll make some error message improvements there instead so that this branch only makes style changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair.
See radiocosmology/draco#258