-
Notifications
You must be signed in to change notification settings - Fork 156
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
Support expression as message in assert #2065
Conversation
12fe893
to
12be820
Compare
d: f64 = 0.4 | ||
e: f64 = 2.5 | ||
f: f64 = 1.0 | ||
assert abs((d * e) - f) <= 1e-6, abs((d * e) - f) |
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.
Shouldn't the second argument be a string? According to the docs: https://docs.python.org/3/reference/simple_stmts.html#grammar-token-python-grammar-assert_stmt, it says that assert expression1, expression2
is equivalent to:
if __debug__:
if not expression1: raise AssertionError(expression2)
And I thought you put in a string into an exception constructor.
In any case, using strings is the most common I think, so I would at least test that, to make sure strings work.
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.
It seems expressions are also supported (since the added tests in this PR with expressions as assert message work with CPython).
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.
I updated the test expr_18
in TEST: Add test for expr as assert message to print a string on assert failure.
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.
I updated the test expr_17
to have a string message with assert.
Otherwise I think this looks good. |
12be820
to
a42dd0c
Compare
a42dd0c
to
c903be8
Compare
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.
I think that looks good, thanks!
towards #2061 (comment), #2054