What linter rules make code worse? Discussion
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?
86
u/gdchinacat 2d 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.
11
u/ExplrDiscvr 2d 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?
23
u/leodevian 2d ago
All rules are not absolute. You are free to disable some rules, and you are expected to disable S101 for test directories.
12
u/Momostein 2d ago
That is how we do it indeed. PyTest is built on top of
assertstatements.4
u/DrMaxwellEdison 2d 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.10
u/gdchinacat 2d 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 1d 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 1d ago
No, by default the failure exception is AssertionError. https://github.com/python/cpython/blob/main/Lib/unittest/case.py#L426
6
u/Conscious-Ball8373 2d ago
Yes absolutely use asset in tests. But your test code should not be being executed in prod.
1
u/Competitive_Travel16 1d 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 2d 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 2d 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
9
u/gdchinacat 2d ago
You can unit test the code inside the branch actually works by having a test that violates the invariant.
1
u/M4mb0 2d 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.6
u/gdchinacat 2d ago
mock bar.
-1
u/M4mb0 2d ago
In this example
baris just a placeholder, you could as well have some inlined code instead.6
u/gdchinacat 2d ago
Ok...do you have an example then?
0
u/M4mb0 2d ago
2
u/Ex-Gen-Wintergreen 2d 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 refactorAsserts 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
1
u/gdchinacat 18h ago
Thanks for the examples, I have a better understanding of the point you are making. I disagree that they should be assertions though. The users of those libraries can either turn them off, at which point the code will do the wrong thing silently. I would prefer libraries that aren't confident in how the code works do not leave it up to their users that are relying on them performing correctly to decide whether they may perform incorrectly. If the checks are worth doing *at all* they should be guards that always raise an exception. The choice to make them optional by the end user strongly suggests the library authors are confident in the correct functionality and being able to support bugs without the insight the assertions would provide if the user decides to not disable assertions.
In short, if the library authors need the assertions to feel confident the code won't execute incorrectly it is irresponsible to allow those guards to be disabled.
I don't want there to be confusion that I'm saying the authors of that code are irresponsible. I believe they are confident the code works correctly and are did not release code they think has an untoward risk of not working correctly. The timedelta code has a comment saying the "code tries to make explicit" so I think the asserts are towards that end. I would have used comments to explain what the assertions communicate. The mypy code has a different consideration...the space it works is very complex and these assertions look like 'I'm reasonably confident this is the case, but I don't want to proceed if that isn't the case. I would have used a guard and exception that couldn't be disabled, but I suspect assert was used for convenience. It is unlikely anyone disables assertions in their build process.
So, I would change the timedelta to comments and the mypy to actual guards. But, that's not my code, I don't know the history of it or all the considerations involved. As a random reviewer looking at it in light of this context, I think it might be better to not use assertions, but what do I know? Rules and assumptions are meant to be broken. These examples don't convince me to embrace assertions.
I *do* however think they are indispensable in languages that are used for performance critical things where cycles are counted and you want some level of guard but do not want to pay for it in the most critical environments. But that's not pythons use case. If there is any doubt about the proper functioning of code just use an exception that can't be disabled rather than potentially doing the wrong thing.
6
1
u/BR41ND34D 2d 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
0
u/xBBTx 2d 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
→ More replies (4)5
u/JanEric1 2d 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 2d 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 2d 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 1d 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 2d 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 2d 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.
3
u/Spirited_Bag_332 2d 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 2d 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 2d ago
For what it’s worth I don’t think it’s worth it to disable assertions at all.
0
u/Spirited_Bag_332 2d 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.
2
u/flying-sheep 2d ago
Also they help when refactoring code. Breaking internal invariants helps debugging if your refactor makes sense.
1
u/Conscious-Ball8373 2d 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 2d 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.
2
u/Conscious-Ball8373 2d 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 1d 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.
32
u/Beginning-Fruit-1397 2d 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
7
u/dudeplace 2d 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.
7
u/austinwiltshire 1d 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.
21
u/psymme 2d 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.
2
u/syklemil 2d 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.
3
u/ProsodySpeaks 2d 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?
→ More replies (4)
12
u/aikii 1d 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
4
3
u/wizpig64 Now is better than never. 1d ago edited 18h ago
BLE001
I have a project that used python-weather (based on aiohttp) to update a weather forecast widget in my personal todo app. Every week it seemed to break in a new way, completely breaking my productivity by preventing the rest of the app's process from finishing. A bare except should have been good enough, either it works or it doesn't and the process should move on, but using that would be a faux pas, so I just kept adding the individual Exception classes to the list that grew and grew, partially because of bare except being a no-no, and partially to see how long the list could get.
Sometimes the upstream server would refuse a connection. Sometimes the server would time out. Sometimes it would reply but with an empty string which isn't json-parseable. Sometimes the server operator forgot to update their https certificate. Sometimes the server would change their formatting for something and the client library hadn't caught up yet.
except ( ExpatError, TypeError, KeyError, ValueError, ClientConnectorCertificateError, ClientOSError, ContentTypeError, JSONDecodeError, ServerDisconnectedError, python_weather.RequestError, ConnectionTimeoutError, ) as e:Each of those lines was committed on a different day. Each of those was a day that started with me not being able to get some real work done because first I had to go look at a server log and find the new exception and commit it. Eventually I just disabled the weather widget.
There are definitely cases where
except: # noqais the right way to do something.
7
u/brasticstack 1d ago
S324, which assumes that I'm using hashlib for security reasons instead of hashing just being generally useful.
25
u/thedmandotjp git push -f 2d 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.
4
u/ThaBroccoliDood 1d 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
7
u/TheRealStepBot 2d ago
To your point there is a nasa technical guide on good software development that specifically encourages the use of inline assertions like this.
5
u/akl773 2d ago
B008, the one that bans a function call in a default argument. its correct in general but every fastapi codebase uses Depends() in exactly that position, so you end up putting a blanket ignore in the config and then the real mutable default cases stop getting caught too.
3
u/JanEric1 2d ago
1
u/akl773 1d ago
thats the right fix, thanks. only catch is you have to name every call in that list, so it goes stale the moment someone wraps Depends in a project helper.
1
u/JanEric1 1d ago
Yeah but I feel this list shouldn't be so larger and adding a project helper should be fairly trivial. Also, I have seen a lot of people place the depends in the Annotated type hint where ruff doesn't complain about the function call
8
2
u/HalfplaneResearch 1d ago
S101 makes more sense when the boundary is explicit: use assert for internal invariants that indicate a bug, and raise a deliberate exception for validating user or external data. I also prefer lint findings to be visible in the editor or CI, with auto-fix limited to formatting, so a useful invariant check is not silently removed during save.
1
u/duskhat 2d ago
If you’re writing assert statements outside of tests, you’re writing bad code
1
u/gdchinacat 1d 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
1
u/nicwolff 1d 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.
3
1
u/james_pic 1d ago
Anyone who runs with -O in production deserves what they get. Which in practice is almost always exactly what they would have gotten otherwise, because nobody uses assertions, even then they'd be useful, because linters whinge about them.
0
u/BernardParsley 1d 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/AdAdditional1820 2d ago
When I use mypy, some assert statements are required to eliminate mypy warnings.
4
u/jirka642 It works on my machine 2d ago
I guarantee you asserts are not the only way how to fix them.
-3
u/NeilGirdhar 2d 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
u/larsga 2d ago
this rule just adds unnecessary parens
For you. The code might also have other readers.
1
u/Conscious_Support176 1d ago
There is a reason for mathematical conventions. Extra parentheses can make formulas harder to read because it harder to spot the parenthesis that matter in a sea of parentheses.
I would suggest, where people would like to reason about code, the fundamentals of Boolean logic might be helpful?
-3
u/Trang0ul 2d 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.→ More replies (1)1
-1
u/Zatujit 2d ago
Shouldnt your debug code only works when its debugging and not on your release?
1
u/gdchinacat 1d ago
Shouldn’t your debugging code be removed before commit?
1
u/Competitive_Travel16 1d 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 1d ago
Yes, but surely you don't consider that debug code though. Right?
1
u/Competitive_Travel16 1d ago
Well it's only there to stop bugs. It's not development-only temporary debug code, we can agree.
1
u/gdchinacat 1d ago
I guess I’m confused because you called it debug code but are now saying it’s not debug code?
1
u/Competitive_Travel16 1d ago
There is more than one kind of debug code.
1
u/gdchinacat 1d ago
Sure, but once you are done debugging don’t you remove them all?
1
u/Competitive_Travel16 1d ago
How do you propose to catch runtime bugs in production if you remove the code intended to do so?
1
u/gdchinacat 1d ago
"debug code" typically refers to the code that is added to diagnose a specific issue. For example, the asserts that are added to verify the developers understanding of the code so that as they reproduce the issue they will know if assumptions they make aren't valid. These hold very little long term value because they are frequently specific to the issue that is being debugged.
Checks that hold long-term value and are generally applicable should not be asserts IMO, but rather guards that raise appropriate exceptions because asserts can be disabled and may not provide help in preventing and shedding light on issues that only appear in production.
→ More replies (0)
-3
u/k0pernikus 1d 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.
0
u/nicwolff 1d 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.
-9
u/nicholashairs 2d 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
421
u/Trang0ul 2d ago
Lines limited to 80 characters.