clj-commons/camel-snake-kebab

Should conversion functions return nil on nil input?

Opened this issue · 2 comments

If we supply nil to a conversion function, it throws an AssertionError

(csk/->kebab-case-keyword nil)
Execution error (AssertionError) at camel-snake-kebab.core/->kebab-case-keyword (core.cljc:21).
Assert failed: (clojure.core/not (clojure.core/nil? s__20671__auto__))

(cske/transform-keys csk/->kebab-case-keyword {nil "bar"})
Execution error (AssertionError) at camel-snake-kebab.core/->kebab-case-keyword (core.cljc:21).
Assert failed: (clojure.core/not (clojure.core/nil? s__20671__auto__))

We had an issue in production where this happened w/ some unexpected input and since it is an Error and not an Exception some things we're not handled properly.

Does it make sense to return nil on nil input? This feels a bit more idiomatic clojure usage to me.

Error : An Error “indicates serious problems that a reasonable application should not try to catch.”
Exceptions : An Exception “indicates conditions that a reasonable application might want to catch.”

At the very least throwing IllegalArgumentException or something may be better than an AssertionError?

qerub commented

Thanks for opening this issue.

I agree that it is unfortunate that an AssertionError is thrown in this situation.

While I personally dislike how nil is tunneled through many Clojure functions and am bothered by some inconsistencies I agree that returning nil would be idiomatic for the "type-preserving" functions. The "type-converting" functions like ->kebab-case-keyword state that they return a keyword so I am not so sure about returning nil for them.

qerub commented

The library has been changed to not throw AssertionError any more. See #70 and #64 for more details. I'm leaving this issue open since I don't think the last word has been said.