clojure-emacs / refactor-nrepl

nREPL middleware to support refactorings in an editor agnostic way
Eclipse Public License 1.0
257 stars 69 forks source link

Honor clj-kondo `:unused-namespace` config, if present #361

Closed vemv closed 2 years ago

vemv commented 2 years ago

Context: https://github.com/clojure-emacs/refactor-nrepl/issues/359#issuecomment-1019639865

Proposes to read .clj-kondo config in search of :unused-namespace config which is essentially the same as our :libspec-whitelist config option.

This way, refactor-nrepl can discover existing config, keeping things smart/DRY, and also reducing friction for teams using diverse tooling.

Feedback welcome - not necessarily to merge as-is!

bbatsov commented 2 years ago

I'm fine with the proposal. Just make sure it's documented in the docs as well, so people won't be surprised.

magnars commented 2 years ago

If some alternative to whitelist is useful, let me suggest allowlist.

On Sun, Feb 6, 2022 at 5:56 PM Bozhidar Batsov @.***> wrote:

@.**** commented on this pull request.

In src/refactor_nrepl/ns/libspec_whitelist.clj https://github.com/clojure-emacs/refactor-nrepl/pull/361#discussion_r800206213 :

@@ -0,0 +1,49 @@ +(ns refactor-nrepl.ns.libspec-whitelist

I know how the config value is named, but I don't see any reason to name a namespace after a configuration option. Especially after in the long run it's highly likely that the current name will be deprecated in favor of some alias with a different name.

— Reply to this email directly, view it on GitHub https://github.com/clojure-emacs/refactor-nrepl/pull/361#discussion_r800206213, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACA4OJ6ZNX44EEED5RYCG3UZ2R2XANCNFSM5NCZEMYQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

bbatsov commented 2 years ago

Yeah, that's the most common alternative. Generally I prefer the more specific "allowed-libspecs" or "ignored-libspecs". There's also the part that the name doesn't really imply it's related to clean-ns, so a name like clean-ns-allowed-libspecs would be even better IMO.

vemv commented 2 years ago

, but I don't see any reason to name a namespace after a configuration option.

It would mean a less ample vocabulary one has to learn and fewer mappings to keep in one's head.

Anyway I've renamed the internals to *allowlist. The piece of config itself can be renamed later.

bbatsov commented 2 years ago

Btw, I still don't get why we need a separate ns for this, as there's already a libspecs ns where it seems to me that this would fit. I've never been too fond of overly granular namespaces, unless there are needed to avoid some circular deps.

vemv commented 2 years ago

Yeah that's one of those things that we'll never quite agree on :)

For me an objective decider is that tools.namespace (refresh) works better with fine-grained namespaces, otherwise altering one file can cascade into reloading many more namespaces, which is slower.

bbatsov commented 2 years ago

I get your point, but I've always aimed to optimize code for people not for tools. :-) Anyways, not a big deal.