top | item 44051405

(no title)

davidjfelix | 9 months ago

Unwrap is fine if used sparingly and as mentioned, to indicate a bug, but in practice it requires discipline and some wisdom to use properly - and by that I mean not just "oh this function should be a `Result` but I'll add that later (never).

I think relying on discipline alone in a team is usually a recipe for disaster or at the very least resentment while the most disciplined must continually educate and correct the least disciplined or perhaps least skilled. We have a clippy `deny` rule preventing panics, excepts, and unwraps, even though it's something we know to sometimes be acceptable. We don't warn because warnings are ignored. We don't allow because that makes it too easy to use. We don't use `forbid`, a `deny` that can't be overridden, because there are still places it could be helpful. What this means is that the least disciplined are pushed to correct a mistake by using `Result` and create meaningful error handling. In cases where that does not work, extra effort can be used to add an inline clippy allow instruction. We strongly question all inline clippy overrides to try to avoid our discipline collapsing into accepting always using `unwrap` & `allow` at review time to ensure nothing slips by mistakenly. I will concede that reviews themselves are potentially a dangerous "discipline trap" as well, but it's the secondary line of defense for this specific mistake.

discuss

order

No comments yet.