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

Support expression as message in assert #2065

Merged
merged 4 commits into from
Jun 30, 2023

Conversation

Shaikh-Ubaid
Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid force-pushed the support_expr_in_assert branch 2 times, most recently from 12fe893 to 12be820 Compare June 30, 2023 11:39
@Shaikh-Ubaid Shaikh-Ubaid requested a review from certik June 30, 2023 11:44
d: f64 = 0.4
e: f64 = 2.5
f: f64 = 1.0
assert abs((d * e) - f) <= 1e-6, abs((d * e) - f)
Copy link
Contributor

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.

Copy link
Collaborator Author

@Shaikh-Ubaid Shaikh-Ubaid Jun 30, 2023

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).

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

@certik
Copy link
Contributor

certik commented Jun 30, 2023

Otherwise I think this looks good.

Copy link
Contributor

@certik certik left a 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!

@certik certik merged commit 00b8d59 into lcompilers:main Jun 30, 2023
8 checks passed
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.

None yet

2 participants