astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
28.79k stars 933 forks source link

Autofix S101 #7635

Open kunaltyagi opened 9 months ago

kunaltyagi commented 9 months ago

code: assert x != 0 ruff: 0.0.291 command: ruff check --fix --fixable S101 . error: S101 Use ofassertdetected

This is an easy fix to substitute in the code specially when opted-in by the developer (if not automatically)

charliermarsh commented 9 months ago

What would you suggest as the correct fix?

kunaltyagi commented 9 months ago

If the assert doesn't have a message, a generic message can be used alongside the assert condition. Eg: for assert x != y

if x == y:
  msg = f"Expected: 'x != y', found: x = {x}, y = {y}"
  raise ValueError(msg)

If there's a message accompanying the assert, eg: assert x != y, "some message"

if x == y:
  msg = "some message"
  raise ValueError(msg)

This is what I've observed people replace their code with when faced with this error. The most often item people might want to change is the error type but this default would more than suffice the 80-20 rule

dimbleby commented 9 months ago

AssertionError would seem the more natural substitution, as already done when auto-fixing B011.

kunaltyagi commented 9 months ago

Sure, that does make more sense

cosmojg commented 9 months ago

If this is implemented, might it wreak havoc on tests?

kunaltyagi commented 9 months ago

I don't think so. Asserts in tests are ignored in the projects I've used ruff with

cosmojg commented 9 months ago

Huh, weird! I had to go out of my way to explicitly declare the following in my pyproject.toml

[tool.ruff.per-file-ignores]
"*test_*.py" = ["S101"]

Without that, I get loads of errors about the use of "assert" in tests.

charliermarsh commented 9 months ago

We don't disable any specific rules in tests by default, so something like what you have there @cosmojg is expected and correct.

cosmojg commented 9 months ago

Ah, got it, thanks for the clarification! That makes sense.

It's an interesting philosophical question whether tests are part of the language itself or merely a product thereof. Regardless, it seems that bandit maintains the same behavior upstream so the point is moot. I suppose some users might prefix scripts with "test" to mean that they are tests in some other sense. Also, the Python interpreter itself doesn't differentiate even though the standard library does (see: unittest), and at the end of the day, explicit behavior generally causes fewer problems than implicit behavior.

All of that said, I agree that autofixing with an AssertionError exception is the way to go!

kunaltyagi commented 9 months ago

@cosmojg I found that I was wrong in my guess and all the projects (I work(ed) on) all had the following common items:

[tool.ruff.per-file-ignores]
# Tests can use magic values, assertions, and relative imports
"tests/**/*" = ["PLR2004", "S101", "TID252"]
kunaltyagi commented 7 months ago

Has there been any progress on this?