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

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

Merged
merged 6 commits into from
Feb 7, 2022
Merged

Conversation

vemv
Copy link
Member

@vemv vemv commented Jan 29, 2022

Context: #359 (comment)

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

bbatsov commented Jan 29, 2022

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

@vemv vemv changed the title Honor clj-kondo :unresolved-namespace config, if present Honor clj-kondo :unused-namespace config, if present Feb 6, 2022
@vemv vemv marked this pull request as ready for review February 6, 2022 13:15
@vemv vemv requested review from expez and bbatsov February 6, 2022 13:15
@@ -0,0 +1,49 @@
(ns refactor-nrepl.ns.libspec-whitelist
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid the problematic "whitelist" terminology. I guess the namespace can be simply named "libspec".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nice thing about the name is that it maps 1:1 to the existing piece of config.

FWIW I also favor avoiding these terminologies where I can, but often keeping things maintainable/non-broken takes precedence.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@magnars
Copy link
Contributor

magnars commented Feb 6, 2022 via email

@bbatsov
Copy link
Member

bbatsov commented Feb 7, 2022

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
Copy link
Member Author

vemv commented Feb 7, 2022

, 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.

@vemv vemv merged commit fb975c6 into master Feb 7, 2022
@vemv vemv deleted the kondo-integration branch February 7, 2022 23:32
@bbatsov
Copy link
Member

bbatsov commented Feb 8, 2022

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
Copy link
Member Author

vemv commented Feb 8, 2022

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

bbatsov commented Feb 9, 2022

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

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

Successfully merging this pull request may close these issues.

3 participants