clojure-emacs / orchard

A fertile ground for Clojure tooling
Eclipse Public License 1.0
326 stars 54 forks source link

[inspector] Use preconditions that throw exceptions instead of errors #270

Closed alexander-yakushev closed 6 months ago

alexander-yakushev commented 6 months ago

Misconfiguration throwing AssertionErrors makes them slip through cider-nrepl's with-safe-transport.

Overall, assertions are bad when integrating with other code.

vemv commented 6 months ago

I'd be happy as well with fixing with-safe-transport - seems fairly hard to guarantee across projects/time that we'll never use assertions.

alexander-yakushev commented 6 months ago

Agreed – additionally handling assertion errors is reasonable in with-safe-transport, but not blanketly catching Throwable (which is a bad idea).

bbatsov commented 6 months ago

Do we even need those preconditions?

alexander-yakushev commented 6 months ago

I'd say yes. For example, setting certain values to zero can hang the inspector or raise very low-level errors. Since those values can ultimately come from the user, we have to validate them somewhere. And I don't like the idea of repeating the validation on each layer (orchard, cider-nrepl, cider); I'd rather do that in a single place that knows what the values should look like (orchard) and flow the helpful errors back through the other layers.

bbatsov commented 6 months ago

I just assumed that bad data would result in exceptions, not lock-ups. If that's not the case - I guess we need those assertions. In general I've rarely found them useful in my work with Clojure, that's why I'm somewhat skeptical of their value.