-
Notifications
You must be signed in to change notification settings - Fork 31
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
QA and CI: Format code using ruff. Validate using ruff and mypy. #660
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.
Reformatting always gives you huge PRs.
""" | ||
assert len(tz) == 5, f"Time zone '{tz}' is given in invalid UTC offset format" | ||
# TODO: Remove use of `assert`. Better use exceptions? |
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.
Only if you will catch the exception where you call _timezone_from_utc_offset
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.
On this spot, I am referring to the fact that the ruff linter discourages the use of assert
statements in code:
S101 Use of `assert` detected
-- https://docs.astral.sh/ruff/rules/assert/
That's why it needs to be suppressed now, alongside a few other spots in this patch, using # noqa: S101
.
My proposal would be to rewrite the assert
statements to constructs like that:
if len(tz) != 5:
raise ValueError(f"Time zone '{tz}' is given in invalid UTC offset format")
I think that would be the correct approach to modernize the code base. wdyt?
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.
Now it makes sense, and I agree that raising an exception is a better option.
About
Rationale
Before releasing 1.0.0, we would like to get the shape of the repository in order. This patch is one in a series to support that.
Details
No other functional changes inside.
References
tests
#659