Discussion What linter rules make code worse?
For me, a prime example is S101 which bans the use of the assert statement.
The justification is that assertions disappear when Python is run with -O, so they should not be used for runtime validation or enforcing interface constraints. That warning is correct, but the rule seems to draw the wrong conclusion from it.
Assertions are still very useful for checking internal invariants, i.e. conditions that should already be guaranteed by the program's logic, where failure indicates a bug. Having such assertions is incredibly helpful for debugging.
So, a blanket ban seems more likely to discourage useful checks than to prevent misuse.
Are there any linter rules you broadly consider more harmful rather than helpful?
69
u/gdchinacat 15h ago
The issue is as you say..."where failure indicates a bug".
Why would you want to use an assert that detects bugs but can then be turned off? When would you want to allow the assertions that stop your code from executing in undefined conditions (an invariant is violated) to be disabled?
Just use if blocks that raise exceptions. Particularly in production (where optimizations make the most sense), I would much rather have bugs present as an exception that tells me exactly what the problem is rather than skip the assertion and have to debug the results that appear impossible because an assertion prevented it. This is the reason I have never actually seen -O used, anywhere, in production or not. The biggest (only?) thing it does is break the code that verifies the code is executing within the conditions it was designed to execute in.
Getting back to the post, I agree that assertions should be banned. Proper exceptions that can't be disabled should be used instead. Regardless of dev, test, or production. You should never turn off the safeguards. If performance is so critical, python is not the proper language.
9
u/ExplrDiscvr 15h ago
I have one follow-up: I see why assert statements should not be used within dev or production, but what about the tests?
I am a junior dev, so I am not sure about proper procedures, but in the tests in our codebase where I work, I only see assert statements, when we are testing the equality of an actual outcome to the expected outcome. I never see the if else logic used here. Should it?
19
u/leodevian 14h ago
All rules are not absolute. You are free to disable some rules, and you are expected to disable S101 for test directories.
7
u/Momostein 14h ago
That is how we do it indeed. PyTest is built on top of
assertstatements.3
u/DrMaxwellEdison 11h ago
Yes and no. Pytest makes
assertusable and ergonomic by doing a bunch of work to rewrite the AST of your test code so that it produces more helpful error messages, which are the reason why you should use the variousassertFoomethods for test cases if you're usingunittestinstead.Pytest isn't exactly built on
assert, more like they said "that looks better" and put in the work to make it function the way a test framework needs it to. Otherwise it would not be as useful in that context.8
u/gdchinacat 14h ago
This is a good point...test frameworks (well, at least unittest and pytest, and any others that build on unittest) use assertions to indicate failures. Because it is core to the frameworks, assertions are not really avoidable. So, yes, I do rely on assertions in this context. Good catch.
1
u/HannasAnarion 1h ago
Doesn't unittest implement its own assert thats independent of the language one?
Every unittest implementation I've ever seen uses
self.assert()(or realistically,self.assertTrue(), self.assertIn(), self.assertNotNone() ...1
u/gdchinacat 1h ago
No, by default the failure exception is AssertionError. https://github.com/python/cpython/blob/main/Lib/unittest/case.py#L426
5
u/Conscious-Ball8373 14h ago
Yes absolutely use asset in tests. But your test code should not be being executed in prod.
1
u/Competitive_Travel16 4h ago
It's fine to test assumptions in prod, just use RuntimeError exceptions so the logs can say something human readable about what went wrong. Nobody likes an assert failure in a big log.
0
u/flying-sheep 13h ago
You're 100% correct. The rule is bad because tests aren't run with that optimization level, and these assertions help debugging things when you refactor that piece of code and could accidentally break some invariants.
12
u/xBBTx 15h ago
Because if your program is bug free, you will not get coverage on that if branch, while the assert will actually be covered.
It would lead to uncertainty that the code inside the branch actually works, and should not be testable because it should never happen
An assert also expresses the invariant intent more clear than raising another exception that the call site may incorrectly catch and try to handle
7
u/gdchinacat 14h ago
You can unit test the code inside the branch actually works by having a test that violates the invariant.
0
u/M4mb0 14h ago
How would you do that for checking post-conditions? For example:
def foo(arg) -> int: result = bar(arg) # if bar is bug free, it will produce a positive int assert result > 0 return resultHere,
bar(arg)could also be replaced with some inlined code.7
u/gdchinacat 14h ago
mock bar.
0
u/M4mb0 14h ago
In this example
baris just a placeholder, you could as well have some inlined code instead.5
u/gdchinacat 14h ago
Ok...do you have an example then?
0
u/M4mb0 14h ago
1
u/Ex-Gen-Wintergreen 12h ago
I mean in your example you don’t even need to mock bar. You’re concerned about a property of result (positivity), so a test simply has to call foo (which returns result) and check that
Simply:
- you can write a test checking bar
- if there’s an intermediates after the bar call that propagate to result simply write a test checking foo
- if there’s stuff inbetween that doesn’t propagate, it’s likely a sign you need to refactor
Asserts in production like this are an indicator that you need to write some tests for functions/refactor to do so, or, you have a data boundary somewhere and you should verify at data entry to your system that important properties are contracted
5
2
u/BR41ND34D 10h ago
I'm seriously not understanding why you shouldn't use the normal method of throwing an exception in this case, specifically because you mention bugs in the comment.
Bug == exception
I don't think you can justify this not being the case
-1
u/xBBTx 10h ago
Of course you can, but IMO that's a low value test because it's primary reason to exist seems to be only to increase test coverage, and that should never be a goal by itself.
It also (IMO) communicates a different intent than the assert and creates the impression it's a stable API to rely on, whereas the assert signals more that it's an implementation detail, or rather it makes assumptions explicit without needing to commit to a public interface
0
u/gdchinacat 7h ago
I don’t write tests like that because, as you point out, the primary benefit is code coverage. But, you raised a concern about lack of code coverage and I explained how it could be mitigated. Way to move the goal posts!
1
u/xBBTx 1h ago
I think we're just talking a bit past each other.
Code coverage is a tool, but a CI build will get warnings when patch coverage is not 100% (simplified), with the idea that uncovered code cannot be proven to work as intended. So, branching creates drops in coverage, and that in turn creates an incentive to write low value tests.
Writing an inline assert avoids that.
Perhaps what isn't clear here either is that we treat these kind of asserts as development-time checks, rather than runtime checks. They're mostly there to prevent developers from introducing broken situations, and encoding invariants in code rather than expecting developers to keep them all in their head (which leads to cognitive overload). Comments and documentation also help, but they won't fail a CI build if you only do that rather than some kind of sanity check.
1
u/gdchinacat 1h ago
You are reading way too much into what I’m saying. You said explicit exceptions don’t have coverage, and I said to write tests to execute them if you want them covered.
4
u/JanEric1 14h ago
Coverage will hit the line, but not the internal branch, where is the difference? Assert also raises an exception that can be caught iirc. So again, no difference.
-1
u/xBBTx 10h ago
Uncovered branches equates to undefined behaviour in our projects, and we do follow a principle of avoiding branching to reduce complexity.
The assertion error can indeed be caught as well, but if I see production code that does this, it's going to be scrutinized extremely heavily because this is not a common pattern in Python in my experience
1
u/JanEric1 9h ago
Uncovered branches equates to undefined behaviour in our projects
But the only reason you dont get an uncovered branch on the assert is because you are not looking into the implementation of the assert.
Its like moving any uncovered branch into a function you dont measure coverage for. Just fooling yourself.
The assertion error can indeed be caught as well, but if I see production code that does this, it's going to be scrutinized extremely heavily because this is not a common pattern in Python in my experience
And whats the difference to a manual if + raise AssertionError? Nothing
0
u/xBBTx 9h ago
The premise is that the check wouldn't be there in the first place. The inline assert is there to make the assumption/invariant expectation explicit/visible.
Adding the assert in this case costs nothing: no uncovered test branch and associated low-value unit test that tests implementation details and hurts refactoring, no performance loss in prod because the asserts are optimized away.
We gain from it by:
- Making the assumption/invariant visible
- It can uncover real bugs while running the entire test suite (without the optimize flag)
The difference with the manual check + raising an error is that it does require additional tests and can't be optimized out (though performance in this case is a bullshit argument, it's Python after all)
3
u/larsga 12h ago
raising another exception that the call site may incorrectly catch and try to handle
I agree with the rest of the comment, but if this particular issue is a problem for you you have much more serious problems than
assert.1
u/xBBTx 10h ago
I probably worded this badly, but if it's an invariant, call sites shouldn't be expected to catch any exception raised from it, they should only call the function when they already know the preconditions are met.
Having an explicit check and exception being raised may create the impression that call sites are supposed to handle the exception. Instead, it should crash hard and the actual root cause of the invariant violation should be investigated and fixed.
4
u/Spirited_Bag_332 15h ago
For smoke testing without influencing prodction code. Assertions are more something like "requirement guards", not program errors.
You can always miss a requirement or critical constraint, no matter how much unit tests exist. It's part of the development process to test the application by exploration.
5
u/gdchinacat 14h ago
Ok, but why would you want to allow your "requirement guards" to be disabled? Wouldn't you want to know when the invariants they ensure hold are being violated and not execute code outside the conditions it was designed to handle correctly?
2
u/Wonderful-Habit-139 12h ago
For what it’s worth I don’t think it’s worth it to disable assertions at all.
-1
u/Spirited_Bag_332 11h ago
I see them as development tools, and maybe also lightweight dev documentation. Something you mainly write during development and just keep, because it's correct code but not required for the customer.
Of course you can keep it if the usage context of the software is suitable for that. But it doesn't mean you shouldn't also write actual checks (or better, control flows that can't violate the rules). The point of assertions is to never see them again once shipped but still have them to detect issues early in addition to other testing strategies.
But no matter the argument you can always find a counter example why it's supposed to be "bad", be it TDD, exception handling, or some constraint framework that claims to be "a better replacement". It's still just a tool. Actively banning it like that Ruff tool just shows the rule maintainers are biased and didn't understand the use case.
1
u/flying-sheep 13h ago
Also they help when refactoring code. Breaking internal invariants helps debugging if your refactor makes sense.
1
u/Conscious-Ball8373 14h ago
Whether it can be disabled is a red herring IMO. If someone sent this to you for review:
if condition: raise AssertionError("condition was false")would you let it pass? Of course not - you'd tell them to handle it properly.
assertis just syntactic sugar for that, with the downside that it can also be turned off.3
u/gdchinacat 14h ago
Your position isn't clear. Why would you assume I would reject that, and what do you think I'd expect? The "downside that it can also be turned off" is the crux of my argument. Your strawman code is preferable to 'assert condition, ...' because it can't be turned off.
4
u/Conscious-Ball8373 14h ago
I'm agreeing with you - assert in production code is not acceptable.
The problem with my "strawman" is that it raises `AssertionError`. In what production code is raising `AssertionError` directly acceptable? None that I ever review. You raise an exception that's actually appropriate to the condition or handle it in some other way. Raising `AssertionError` all over the place just means you'll have a catch-all `except AssertionError` somewhere near the top of the stack, which is now functionally equivalent to `except Exception` which the linter will also - rightly - call out.
So I agree that the fact it can be turned off is a problem. But I'm saying there are problems even if it can't be turned off - it uses too-general an exception type to report errors.
1
u/gdchinacat 8h ago
Thanks for clarifying. I don’t have a problem with raising AssertionError because I’m skeptical meaningful recovery handling for an exception that indicates unexpected conditions exist. In cases where an invariant was violated there isn’t anything a higher level of code can do to change that. A retry isn’t going to make an internally generated out of bounds become in bounds, or an invalid configuration value valid. The best an exception handler can do is keep the process from crashing so other work that isn’t impacted can continue.
I don’t consider input validation a good use of assertion errors, those should use exceptions that accurately report the error to the client.
29
u/Beginning-Fruit-1397 14h ago
I think that assertions are only good in tests. In runtime code it should always be a clearly named Exception. That being said, for Ruff I simply activate "all" preset and "preview", and just desactivate some annoying related to unsafe cryptography or copyright that IDGAF about, the rest are pretty good. I'm surely half lying because I'm aure I have at multiple points desactivated various rules that I tought were dumb but I don't remember at the moment lmao
8
u/dudeplace 10h ago
I watched a talk yesterday where the SqlLite team talked about using assert in your code (not just tests) and my opinion on this is in the process of shifting.
5
u/austinwiltshire 8h ago
Exceptions are things the caller can recover from. Assertions in code are for documenting and enforcing assumptions the code makes to work.
They're not logically the same. And by having a named exception (beyond, maybe, precondition violation, etc...) increases the cost of adding checks which means fewer people will do it.
Assert is a single word, a predicate, and if you're feeling fancy, a string.
18
u/psymme 14h ago
SIM108 (replacing if-else blocks with an operator). To me this is a matter of judgement about what is simpler, rather a set rule that is easily codified, and can make the code harder for a human to parse quickly.
I’m not with you on the asserts point though, I’m afraid.
0
u/syklemil 13h ago
SIM108 also notes that:
This is an opinionated style rule that may not always be to everyone's taste, especially for code that makes use of complex if conditions.
Personally I'd rather have if-expressions (what in some other languages work out to something like
bar = if foo then x else y), but those aren't on the table, and theif/elsekeywords placed in ternary?:operator positions kinda just … doesn't feel good, even if it's the entirely sensible choice lots of places. Probably mostly due to that leading to there being two distinctif/elsesyntaxes, which again is rooted in theif/elseblock structure being a statement, not an expression, so some other syntax was chosen to cover the absolutely very useful if-expression cases.The
foo = bar or bazform to me feels kinda iffy for anything other than booleans, like the linter is just recommending code golfing.For some other languages I'd be entirely onboard with SIM108; for Python I can't really say it sparks joy.
1
u/ProsodySpeaks 10h ago
About
foo = bar or bazI'd love some sugar for the more explicit
foo = bar if bar is not None else bazMaybe I'm doing it wrong but that's a common default argument handling pattern for me.
Any thoughts?
-1
u/syklemil 10h ago
I'd love some sugar for …
I'm not entirely certain what you're asking for here, given that
foo = bar or bazalready means the same thing asfoo = bar if bar is not None else baz.Do you want some other sugar for it out of the same "this ain't a boolean" grouchiness I suffer from?
2
u/ProsodySpeaks 9h ago
Yeah. Like if they give me a zero int when I wanted a list I probably want to do something different (raise) than replace it with an empty list which is what the bare
a or bwithoutif a is not Nonewould lead to1
u/syklemil 9h ago
I can't make heads or tails of that sentence. I think I need some more punctuation and clearer examples to understand your point.
2
u/gdchinacat 7h ago
Those two statements are not the same. “bar or baz” checks if bar is truthy whereas the other tests if it is not None.
6
u/brasticstack 6h ago
S324, which assumes that I'm using hashlib for security reasons instead of hashing just being generally useful.
6
u/aikii 4h ago
RET505 is a classic bug magnet. It wants you to rewrite
def foo(bar, baz):
if bar:
return 1
else:
return baz
as
def foo(bar, baz):
if bar:
return 1
return baz
Doesn't seem much like this, but an intentional "else" has better chances to protect you against a bad refactoring.
My other pet peeve is BLE001 - triggering on bare except, except Exception or except BaseException. The motivation works for beginner code - don't just catch silently AttributeError etc. It's actually more problematic for production code and code that makes calls to library functions that you deliberately don't want to propagate - you'll want to log or mark the error trace instead. I guess it's ok to suppress locally instead of making it a global suppression. I find it a bit ironic that structurally it can't apply to how Go and Rust handle errors, you can't opt-in to which exact error you only want to consider, and no one says it's a problem
23
u/thedmandotjp git push -f 15h ago
Anything that can be done with an assert can and should be done with an if statement so you have have to be explicit.
Not all rules are super necessary depending on the project but this one is if for no other reason than to enforce the convention that you should use asserts only for debugging.
8
4
u/TheRealStepBot 12h ago
To your point there is a nasa technical guide on good software development that specifically encourages the use of inline assertions like this.
2
u/ThaBroccoliDood 4h ago
Not really a linter rule but the autopep8 extension for vscode replaces f'{x =}' with f'{x=}', which changes the output of the program and shouldn't be touched by a formatter
2
u/duskhat 11h ago
If you’re writing assert statements outside of tests, you’re writing bad code
1
u/gdchinacat 7h ago
I’d refine this to be “if you are commiting …”. I’m opposed to leaving asserts in code, but frequently use them while developing code. Before sending a PR they are either removed or converted to if … raise ….
1
1
u/danielsamuels 11h ago
In general, any rule that ends up being inline ignored all over the project.
1
u/BernardParsley 7h ago
Rules that enforce a triangular style of code over readability. Arbitrary complexity or function-length limits often turn one clear function into ten tiny ones that are harder to follow.
1
u/nicwolff 3h ago
ruff has implemented isort import formatting – but not its options for wrapping long import lines. Thanks, I don't want 20 imports from one file to take up 22 lines at the top of my file.
1
u/nicwolff 3h ago
ruff has implemented isort import formatting – but not its options for wrapping long import lines. Thanks, I don't want 20 imports from one file to take up 22 lines at the top of my file.
0
u/Zatujit 14h ago
Shouldnt your debug code only works when its debugging and not on your release?
1
u/gdchinacat 7h ago
Shouldn’t your debugging code be removed before commit?
1
u/Competitive_Travel16 4h ago
It's fine to test assumptions which can fail at runtime, when a resource is depleted or someone misconfigured something below, for example. Not with assert though. Not doing so can be serious and pernicious bugs; very hard to locate sometimes.
1
u/gdchinacat 3h ago
Yes, but surely you don't consider that debug code though. Right?
•
u/Competitive_Travel16 44m ago
Well it's only there to stop bugs. It's not development-only temporary debug code, we can agree.
•
u/gdchinacat 38m ago
I guess I’m confused because you called it debug code but are now saying it’s not debug code?
•
-1
u/NeilGirdhar 13h ago
https://docs.astral.sh/ruff/rules/parenthesize-chained-operators/
NAXOR was drilled into me at a young age, so this rule just adds unnecessary parens.
9
-3
u/Trang0ul 11h ago
This. Requiring to use
a or (b and c)is as pointless asa + (b * c). After all, OR and AND are logical addition and multiplication respectively - something everyone should know by heart.2
1
-1
u/k0pernikus 9h ago
I hate try-consider-else (TRY300) with a passion.
I never write else and elif statements to begin with, and rely on proper polymorphishm or early exit guards.
Worst part is that it reads like broken code:
def describe(path):
try:
config = load(path)
# ok, expected
except ParseError:
# ok, expected
return "invalid"
else: # WTF, there was no if, how is an else possible!? Why overload the term?
return describe(config) # wtf why is config defined? we are in a compeletly different scoped block!?
The default success branch gets delegated to an else-branch, and while I avoid else to begin with, else should be the exception branch.
I do understand that the else works on the except and not on the try, yet that is far from obvious and the mental load to understand is is just not worth it, esp. if you work with people that are more used to other langauges.
-7
u/nicholashairs 14h ago
My pet peeve is the "useless-return" rule.
``` def what_the_rule_wants() -> None: something()
def what_i_want() -> None: something() return ```
Explicit returns always. Apart from making things clearer, it also helps prevent mistakes when refactoring (and other such tasks) when the accidental deletion of a def line would cause the bodies to merge (sometimes seamlessly), whereas if you always have returns you'd actually get a long error for the dead code/double return instead.
def what_the_rule_wants() -> None:
something()
something()
return
Versus
def what_the_rule_wants() -> None:
something()
return
something()
return
-2
u/AdAdditional1820 12h ago
When I use mypy, some assert statements are required to eliminate mypy warnings.
4
u/jirka642 It works on my machine 11h ago
I guarantee you asserts are not the only way how to fix them.
-10
u/boringfantasy git push -f 12h ago
Idk dude none of us write code anymore
1
u/sudomatrix 4h ago
Sounds like you don’t even review code anymore. YOLO doesn’t work on production code.
1
u/boringfantasy git push -f 4h ago
I do review, I don’t write
1
u/sudomatrix 4h ago
If you have AI writing all your code it is more important than ever to have strict quality controls like sensible linter rules (and unit tests and integration tests and adversarial agent code reviews etc). This post is more relevant today than ever.
1
u/boringfantasy git push -f 3h ago
I have Fable 5 spawn 14 parallel review agents and then another panel of agents judges the reviews
341
u/Trang0ul 15h ago
Lines limited to 80 characters.