Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should conversion functions return nil on nil input? #63

Open
vincentjames501 opened this issue Apr 12, 2020 · 2 comments
Open

Should conversion functions return nil on nil input? #63

vincentjames501 opened this issue Apr 12, 2020 · 2 comments

Comments

@vincentjames501
Copy link

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
Copy link
Collaborator

qerub commented Apr 26, 2020

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.

vincentjames501 pushed a commit to vincentjames501/camel-snake-kebab that referenced this issue Apr 26, 2020
qerub added a commit that referenced this issue Aug 9, 2020
Preconditions throw an AssertionError if they fail.

Error:s are usually not caught/handled so this can have dire
consequences if an unexpected nil value shows up (see #63).

This reverts commit 4b3d730.
qerub added a commit that referenced this issue Aug 9, 2020
Preconditions throw an AssertionError if they fail.

Error:s are usually not caught/handled so this can have dire
consequences if an unexpected nil value shows up (see #63).

This reverts commit 4b3d730.
@qerub
Copy link
Collaborator

qerub commented Aug 9, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants