Skip to content

extend assert functions by add fail_msg~ argument #2172

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

myfreess
Copy link
Contributor

@myfreess myfreess commented May 27, 2025

use case: calling assert_true/assert_false in for loop unicode test

Copy link

peter-jerry-ye-code-review bot commented May 27, 2025

Inconsistent string interpolation usage in default messages

Category
Maintainability
Code Snippet
fail_msg~ : String = "\{debug_string(a)} != \{debug_string(b)}"
Recommendation
Consider using consistent string formatting across all assertions. For assert_true/false, consider using debug_string(x) like other assertions
Reasoning
The assertion functions use different string formatting approaches. assert_eq/not_eq use debug_string while assert_true/false directly interpolate the value. Using debug_string consistently would provide better formatted output especially for complex types.

Redundant string conversion removed but behavior changed

Category
Correctness
Code Snippet
if x {
fail(fail_msg, loc~)
}
Recommendation
Consider keeping the debug_string conversion in the default message:
fail_msg~ : String = "{debug_string(x)} is not false"
Reasoning
The original code explicitly converted the boolean to string using debug_string. The new version may produce different output for the default message, potentially breaking existing tests that depend on the exact error message format.

Documentation needs update to reflect new parameter

Category
Maintainability
Code Snippet
/// # Examples
/// /// assert_true(1 == 1) ///
Recommendation
Update documentation to include examples of custom failure messages:

/// # Examples
/// ```
/// assert_true(1 == 1)
/// assert_true(1 == 1, "Custom error message")
/// ```

**Reasoning**
The documentation examples don't show the new fail_msg parameter usage. Adding examples would help users understand how to use the new functionality.

</details>

@myfreess myfreess force-pushed the myfreess/extend-assert branch from d803a73 to b6be526 Compare May 27, 2025 07:38
@coveralls
Copy link
Collaborator

coveralls commented May 27, 2025

Pull Request Test Coverage Report for Build 7009

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.521%

Totals Coverage Status
Change from base Build 7006: 0.0%
Covered Lines: 8746
Relevant Lines: 9453

💛 - Coveralls

@myfreess myfreess force-pushed the myfreess/extend-assert branch from b6be526 to 733e691 Compare May 27, 2025 07:39
@myfreess myfreess marked this pull request as draft May 27, 2025 07:40
@myfreess myfreess marked this pull request as ready for review May 27, 2025 07:42
if not(x) {
fail("`\{x}` is not true", loc~)
fail(fail_msg, loc~)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the default is empty

@myfreess myfreess force-pushed the myfreess/extend-assert branch 5 times, most recently from 4455c18 to c52556c Compare May 28, 2025 05:54
@myfreess myfreess force-pushed the myfreess/extend-assert branch from ef44c50 to 25a0dfd Compare May 29, 2025 10:16
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.

4 participants