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

Warn on cond & condp without default case #1164

Closed
neotyk opened this issue Feb 10, 2021 · 6 comments
Closed

Warn on cond & condp without default case #1164

neotyk opened this issue Feb 10, 2021 · 6 comments
Labels
expired expired - might be closed due to too little activity
Projects

Comments

@neotyk
Copy link

neotyk commented Feb 10, 2021

Is your feature request related to a problem? Please describe.
It's perfectly valid to have cond or condp without default case, though exception is thrown when no matching case is found.

Describe the solution you'd like
Warn on cond & condp used without default case.

Assuming this is really wanted I have some OSS time this Friday, just sayin'

@borkdude
Copy link
Member

borkdude commented Feb 10, 2021

Cond doesn't throw an exception without a default case:

user=> (cond (odd? 1) :true)
:true
user=> (cond (odd? 2) :true)

For case we don't lint this as a warning since case without a default case is meant to throw
I did consider this once, but this was met with feedback from e.g. Alex Miller (@puredanger) that case (and I think the same goes for condp) is meant to cover a limited set of cases and should throw when a case is unhandled.

https://twitter.com/puredanger/status/1116696133315760128

Screenshot 2021-02-10 at 12 41 38

So as it stands, I don't think any changes are needed here.

@neotyk
Copy link
Author

neotyk commented Feb 10, 2021

Maybe I was too trigger happy about cond, but condp is throwing:

(condp = 0 1 :something)
;=> Execution error (IllegalArgumentException) at dev/eval60650 (form-init4047019337742748236.clj:1).
No matching clause: 0

@neotyk
Copy link
Author

neotyk commented Feb 10, 2021

Yeah ok, throwing by design. I'd still like the warning for it.

@borkdude borkdude added this to Needs triage in clj-kondo via automation Feb 10, 2021
@borkdude borkdude moved this from Needs triage to Needs hammock time in clj-kondo Feb 10, 2021
@borkdude
Copy link
Member

borkdude commented Feb 10, 2021

@neotyk Currently you can write a custom hook to check for condp without defaults:

$ clj-kondo --lint src
src/program.clj:3:1: warning: condp without default
linting took 14ms, errors: 0, warnings: 1

$ cat src/program.clj
(ns program)

(condp = 1
  2 :foo)

$ cat .clj-kondo/config.edn
{:hooks {:analyze-call {clojure.core/condp hooks/condp-hook}}
 :linters {:condp-without-default {:level :warning}}}
 
$ cat .clj-kondo/hooks.clj
(ns hooks
  (:require [clj-kondo.hooks-api :as api]))

(defn condp-hook [{:keys [:node]}]
  (let [children (rest (:children node))]
    (when (even? (count children))
      (api/reg-finding! (assoc (meta node)
                               :message "condp without default"
                               :type :condp-without-default)))))

Screenshot 2021-02-10 at 13 37 09

@serioga
Copy link

serioga commented Feb 10, 2021

For me the cond without default case is fine and handy.

@stale
Copy link

stale bot commented Dec 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the expired expired - might be closed due to too little activity label Dec 3, 2021
@stale stale bot closed this as completed Jan 2, 2022
clj-kondo automation moved this from Needs hammock time to Done Jan 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
expired expired - might be closed due to too little activity
Projects
clj-kondo
  
Done
Development

No branches or pull requests

3 participants