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

[ruff] Add assert-with-print-message rule (#11974) #11981

Merged
merged 5 commits into from
Jun 23, 2024

Conversation

denwong47
Copy link
Contributor

Summary

Addresses #11974 to add a RUF rule to replace print expressions in assert statements with the inner message.

An autofix is available, but is considered unsafe as it changes behaviour of the execution, notably:

  • removal of the printout in stdout, and
  • AssertionError instance containing a different message.

While the detection of the condition is a straightforward matter, deciding how to resolve the print arguments into a string literal can be a relatively subjective matter. The implementation of this PR chooses to be as tolerant as possible, and will attempt to reformat any number of print arguments containing single or concatenated strings or variables into either a string literal, or a f-string if any variables or placeholders are detected.

Test Plan

cargo test.

Examples

For ease of discussion, this is the diff for the tests:

 # Standard Case
 # Expects:
 # - single StringLiteral
-assert True, print("This print is not intentional.")
+assert True, "This print is not intentional."
 
 # Concatenated string literals
 # Expects:
 # - single StringLiteral
-assert True, print("This print" " is not intentional.")
+assert True, "This print is not intentional."
 
 # Positional arguments, string literals
 # Expects:
 # - single StringLiteral concatenated with " "
-assert True, print("This print", "is not intentional")
+assert True, "This print is not intentional"
 
 # Concatenated string literals combined with Positional arguments
 # Expects:
 # - single stringliteral concatenated with " " only between `print` and `is`
-assert True, print("This " "print", "is not intentional.")
+assert True, "This print is not intentional."
 
 # Positional arguments, string literals with a variable
 # Expects:
 # - single FString concatenated with " "
-assert True, print("This", print.__name__, "is not intentional.")
+assert True, f"This {print.__name__} is not intentional."

 # Mixed brackets string literals
 # Expects:
 # - single StringLiteral concatenated with " "
-assert True, print("This print", 'is not intentional', """and should be removed""")
+assert True, "This print is not intentional and should be removed"
 
 # Mixed brackets with other brackets inside
 # Expects:
 # - single StringLiteral concatenated with " " and escaped brackets
-assert True, print("This print", 'is not "intentional"', """and "should" be 'removed'""")
+assert True, "This print is not \"intentional\" and \"should\" be 'removed'"
 
 # Positional arguments, string literals with a separator
 # Expects:
 # - single StringLiteral concatenated with "|"
-assert True, print("This print", "is not intentional", sep="|")
+assert True, "This print|is not intentional"
 
 # Positional arguments, string literals with None as separator
 # Expects:
 # - single StringLiteral concatenated with " "
-assert True, print("This print", "is not intentional", sep=None)
+assert True, "This print is not intentional"
 
 # Positional arguments, string literals with variable as separator, needs f-string
 # Expects:
 # - single FString concatenated with "{U00A0}"
-assert True, print("This print", "is not intentional", sep=U00A0)
+assert True, f"This print{U00A0}is not intentional"
 
 # Unnecessary f-string
 # Expects:
 # - single StringLiteral
-assert True, print(f"This f-string is just a literal.")
+assert True, "This f-string is just a literal."
 
 # Positional arguments, string literals and f-strings
 # Expects:
 # - single FString concatenated with " "
-assert True, print("This print", f"is not {'intentional':s}")
+assert True, f"This print is not {'intentional':s}"
 
 # Positional arguments, string literals and f-strings with a separator
 # Expects:
 # - single FString concatenated with "|"
-assert True, print("This print", f"is not {'intentional':s}", sep="|")
+assert True, f"This print|is not {'intentional':s}"
 
 # A single f-string
 # Expects:
 # - single FString
-assert True, print(f"This print is not {'intentional':s}")
+assert True, f"This print is not {'intentional':s}"
 
 # A single f-string with a redundant separator
 # Expects:
 # - single FString
-assert True, print(f"This print is not {'intentional':s}", sep="|")
+assert True, f"This print is not {'intentional':s}"
 
 # Complex f-string with variable as separator
 # Expects:
 # - single FString concatenated with "{U00A0}", all placeholders preserved
 condition = "True is True"
 maintainer = "John Doe"
-assert True, print("Unreachable due to", condition, f", ask {maintainer} for advice", sep=U00A0)
+assert True, f"Unreachable due to{U00A0}{condition}{U00A0}, ask {maintainer} for advice"
 
 # Empty print
 # Expects:
 # - `msg` entirely removed from assertion
-assert True, print()
+assert True
 
 # Empty print with separator
 # Expects:
 # - `msg` entirely removed from assertion
-assert True, print(sep=" ")
+assert True
 
 # Custom print function that actually returns a string
 # Expects:
@@ -100,4 +100,4 @@
 # Use of `builtins.print`
 # Expects:
 # - single StringLiteral
-assert True, builtins.print("This print should be removed.")
+assert True, "This print should be removed."

Known Issues

The current implementation resolves all arguments and separators of the print expression into a single string, be it StringLiteralValue::single or a FStringValue::single. This:

  • potentially joins together strings well beyond the ideal character limit for each line, and
  • does not preserve multi-line strings in their original format, in favour of a single line "...\n...\n..." format.

These are purely formatting issues only occurring in unusual scenarios.

Additionally, the autofix will tolerate print calls that were previously invalid:

assert True, print("this", "should not be allowed", sep=42)

This will be transformed into

assert True, f"this{42}should not be allowed"

which some could argue is an alteration of behaviour.

@denwong47 denwong47 changed the title [ruff] Add assert-with-print-expression flag (#11974) [ruff] Add assert-with-print-expression rule (#11974) Jun 22, 2024
@charliermarsh charliermarsh self-requested a review June 23, 2024 15:01
@charliermarsh charliermarsh self-assigned this Jun 23, 2024
@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Jun 23, 2024
@charliermarsh
Copy link
Member

Thanks! I will review.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

This is excellent work, nicely done.

@MichaReiser
Copy link
Member

I think I would rename the rule to assert-with-print-message, it's otherwise unclear if it also captures assert print("test")

@charliermarsh
Copy link
Member

Makes sense to me.

@charliermarsh charliermarsh added the preview Related to preview mode features label Jun 23, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) June 23, 2024 16:51
@charliermarsh charliermarsh merged commit c3f61a0 into astral-sh:main Jun 23, 2024
16 checks passed
@MichaReiser MichaReiser changed the title [ruff] Add assert-with-print-expression rule (#11974) [ruff] Add assert-with-print-message rule (#11974) Jun 23, 2024
@zanieb zanieb added the great writeup A wonderful example of a quality contribution label Jun 23, 2024
@denwong47 denwong47 deleted the assert-with-print-expression branch June 23, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
great writeup A wonderful example of a quality contribution preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants