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

clean-ns: Introduce ^:keep metadata on libspecs #189

Closed
wants to merge 1 commit into from

Conversation

plexus
Copy link
Contributor

@plexus plexus commented Jan 31, 2017

This allows marking up required namespaces that should not be cleaned by clean-ns.

This is useful to require namespaces only so they get loaded for side-effects. It can also be used in (ns user) to make functions available for the REPL.

Example

    (ns user
      (:require [com.stuartsierra.component :as component]
                ^:keep [reloaded.repl :refer [system init start stop go reset]]])

This PR is not ready yet, I'm just at the point where I got it to work, and I'd appreciate feedback.

Questions:

  • Is ^:keep the best key to use? (bikeshedding time!)
  • Right now it's only supported in :require, does it make sense to also have it for :import or :use. (Probably not since you don't usually :import for side effects, and :use is deprecated, but maybe there's an argument to be made).
  • Should it also work on individuals function names in a :refer macro?
  • Should it work on nested vectors when grouping by prefix?

Todo for me:

  • Check if it also works on CLJS/CLJC
  • Update CHANGELOG
  • Update README

Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • 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!

This allows marking up required namespaces that should not be cleaned by
clean-ns.

This is useful to require namespaces only so they get loaded for side-effects.
It can also be used in (ns user) to make functions available for the REPL.

Example

    (ns user
      (:require [com.stuartsierra.component :as component]
                ^:keep [reloaded.repl :refer [system init start stop go reset]]])
parse/read-ns-decl)
meta? (second ns-decl)
ns-name (if (symbol? meta?) meta? (first (nnext ns-decl)))
shorthand-meta? (-> (java.util.regex.Pattern/compile (str "\\^:([^\\s]+)\\s" ns-name))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was too eager, it would transform

(ns user (:require ^:keep [,,,]))

into

(ns ^:keep user)

@expez
Copy link
Member

expez commented Jan 31, 2017

Is ^:keep the best key to use? (bikeshedding time!)

I'd prefer refactor-nrepl/keep, that way it's possible for someone to find out what that metadata means even if they're not familiar with the tool.

Right now it's only supported in :require, does it make sense to also have it for :import or :use. (Probably not since you don't usually :import for side effects, and :use is deprecated, but maybe there's an argument to be made).

Agree with you on this. We can always revisit it later.

Should it also work on individuals function names in a :refer macro?

I don't think that's needed, when you mark it as keep I think it's OK to interpret that as "I'll manage this one manually". I might be biased because I almost never refer stuff.

Should it work on nested vectors when grouping by prefix?

We do prefix re-writing today so we need at least partial support.

(ns my-ns 
  (:require 
    [foo.bar :as bar]
    ^:refactor-nrepl/keep 
    [foo.baz :as baz]))

With prefix rewriting this would look like:

(ns my-ns 
  (:require 
    [foo
      [bar :as bar]
      ^:refactor-nrepl/keep 
      [baz :as baz]]))

But I don't think we need to worry about:

(ns my-ns 
  (:require 
    ^:refactor-nrepl/keep 
    [foo
      [bar :as bar]
      [baz :as baz]]))

If it's easier to just support both, then that's fine I think, but I wouldn't bother adding complexity to support this final variant.

@plexus
Copy link
Contributor Author

plexus commented Jan 31, 2017

I'd prefer :refactor-nrepl/keep, that way it's possible for someone to find out what that metadata means even if they're not familiar with the tool.

That makes sense. I was thinking it could be a convention that other tools can also pick up, but that's probably not too likely anyway.

@expez
Copy link
Member

expez commented Apr 16, 2017

Ping @plexus :)

@plexus
Copy link
Contributor Author

plexus commented Apr 18, 2017

Hey, sorry for letting this sit. I've been completely swamped with other stuff, and given that it's still a bit of work to do this feature properly I won't get to it very soon.

@bbatsov
Copy link
Member

bbatsov commented Mar 20, 2019

I'm closing this PR due to no recent activity. Feel free to re-open it if you ever come back to it.

@bbatsov bbatsov closed this Mar 20, 2019
@aiba
Copy link
Contributor

aiba commented Jun 3, 2019

I would find this really useful. A common challenge for me with clean-ns is when I require a clojure namespace that contains a defrecord, and then also import that record. As in:

(ns foo.core
  (:require [bar.core])
  (:import [bar.core.SomeRecord]))

The require is needed for the runtime generation of SomeRecord, but gets removed by clean-ns.

@slipset
Copy link

slipset commented Jun 18, 2019

Here is the solution that was chosen for clj-kondo, which I think is a very wise choice.

Any :require that doesn't either use :refer or :as is assumed to be there for sideeffecting reasons and thus cannot be removed.

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.

5 participants