search

jonase / eastwood

Clojure lint tool
1.08k stars 66 forks source link

unused-fn-args too aggressive with multimethods #1

Closed amalloy closed 3 years ago

amalloy commented 12 years ago

It's at least somewhat common to have multimethods where not all of the args are used in every case. For example:

(defmulti display (fn [which-display string-to-show]
                    which-display))

Likewise you could have a defmethod that doesn't need to know about some of the args, which are used only for dispatching. I'm aware you can use _ as an argument name in these cases, but having the name available is useful for code readability. I think it would be nice if Eastwood had a way to understand this. For example, maybe this is so common that it's correct to just never check multimethods for unused args? I don't think so, though. But perhaps instead it's a good idea to let the author put metadata in the source saying "Look, I know I don't use this arg, don't bug me about it." Something like:

(defmulti display (fn [which-display ^:lint/unused string-to-show]
                    which-display))

Obviously i haven't thought through this feature request very thoroughly; just sorta bringing to your attention the fact that there's a hole to be filled somehow. Anyway, thanks for Eastwood: I'm eagerly trying it out on some of my projects.

jonase commented 12 years ago

I Agree that :unused-fn-args is a bit aggressive. You can add :eastwood {:exclude-linters [:unused-fn-args]} to .lein/profiles.clj to disable it. I don't think I want to make people litter their code with metadata to help the linter do the right thing. That feels kind of backwards to me. Maybe :unused-fn-args shouldn't be in the set of default linters.

amalloy commented 12 years ago

Perhaps. On that note, it's currently quite hard to add reflection to my list of linters: I can lein2 eastwood '{:linters #{:reflection}}' to get just reflection, but really I'd like to run it along with the usual suspects. Would you be interested in a pull request for something like an :add-linters option?

jonase commented 12 years ago

Would you be interested in a pull request for something like an :add-linters option

Yes, of course :).

I didn't include reflection in the default set of linters because of *warn-on-reflection*.

AlexBaranosky commented 12 years ago

@amalloy I agree. I also like to see the argument name for readability. One convention we've been using in Midje, is to mark unused parameters as either (fn [_meaningful-arg-name_] ...) or as (fn [_meaningful-arg-name] ...)

@jonase Has already accepted my pull request that makes it so that :unused-fn-args will ignore any parameter that begins with an underscore. https://github.com/jonase/eastwood/pull/6