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

Add setting that allows keeping unused unaliased requires #386

Conversation

OknoLombarda
Copy link
Contributor

@OknoLombarda OknoLombarda commented Sep 17, 2022

#292
#359

Add setting that allows keeping unused unaliased requires, disabled by default

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (run lein do clean, test)
  • Code inlining with mranderson works and tests pass with inlined code (run ./build.sh install -- takes a long time)
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)

Thanks!

@OknoLombarda
Copy link
Contributor Author

I'll add tests and update readme if you have no comments regarding the code

@@ -60,7 +61,9 @@
symbols-in-file)
(some (partial libspec-in-use-without-refer-all? libspec)
symbols-in-file))
(libspec-in-use-with-rename? libspec symbols-in-file))
(libspec-in-use-with-rename? libspec symbols-in-file)
(and (not libspec-as)
Copy link
Member

@expez expez Sep 17, 2022

Choose a reason for hiding this comment

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

The example in clj-kondo linters file is (ns foo (:require [foo.specs])).

In other words, this will allow both foo.specs and [foo.specs] to remain, but isn't the wrapping what's used to signal that it's side-effecting in the clj-kondo convention?

Copy link
Contributor Author

@OknoLombarda OknoLombarda Sep 17, 2022

Choose a reason for hiding this comment

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

It doesn't seem like clj-kondo cares about brackets
image
image

@vemv
Copy link
Member

vemv commented Sep 17, 2022

The resolution of #359 (comment) is quite more recent than the one of the other issue.

I'd like refactor-nrepl to contribute to a more explicit, standarized tooling landscape.

The lack of an alias does't mean much in official Clojure terms. The extra semantic is a clj-kondo invention, that precedes its more explicit :unused-namespace config.

So I'd prefer that people write explicit .clj-kondo config. It's something that refactor-nrepl currently reads. It also is something that other tools (e.g. Eastwood) could also eventually read.

It's a much better approach than reading magic comments (#_,,,) or making up semantics. For users and tool makers alike.

@vemv
Copy link
Member

vemv commented Sep 17, 2022

Also, non trivially, one approach is naturally more prone to false positives than the other.

If I add an entry to my .clj-kondo file, then my intent is certain.

Contrariwise, adding [some-lib] without an alias is an everyday action, it can happen for any reason, from any person. So we can easily get a false positive.

For that reason alone, e.g. making our tools correct I think the right approach is clear now. It's not just aesthetics or 'standards'.

Will be moving this PR to draft (with the intent of closing it if there isn't much opposition)

@vemv vemv marked this pull request as draft September 17, 2022 12:11
@OknoLombarda
Copy link
Contributor Author

Yeah, I just though that'd be convenient, but you're right, picking convenience in some situations over correctness is not worth it 【⸟ ͟ʖ⸟】

@magnars
Copy link
Contributor

magnars commented Sep 17, 2022

Any reason this could not be extended to actually using the clj-kondo file? Or is that already supported?

@OknoLombarda
Copy link
Contributor Author

It is already supported #361

@magnars
Copy link
Contributor

magnars commented Sep 17, 2022

Excellent 👍👍

@vemv
Copy link
Member

vemv commented Sep 17, 2022

Thank you both for the understanding! 🍻

@vemv vemv closed this Sep 17, 2022
@OknoLombarda OknoLombarda deleted the keep-unused-unaliased-requires branch September 17, 2022 13:44
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.

4 participants