Don't remove implicitly needed requires from `clean-ns`

[nblumoe]

I think this might be a tricky one, but maybe it can be solved in a sane manner and appropriate time.

In some rare cases, one needs to require namespaces, which are then not explicitly being used in the code. One example would be the popular migration library joplin: https://github.com/juxt/joplin#calling-joplin-from-your-code

Here is the example usage:

(ns example
  (:require [joplin.core :as joplin]
            [joplin.dt.database]))

(joplin/migrate-db
  {:db {:type :dt,
        :url "datomic:mem://test"}
   :migrator "joplin/migrators/datomic"
   :seed "seeds.dt/run"})

[joplin.dt.database] will be removed from the ns declaration when clean-ns is invoked. This results in breaking the code.

Can this be caught? Static code analysis does not allow to distinguish between valid and invalid cases, but might be possible to get it via dynamic inspection. Not sure, would be awesome though.

[expez]

I don't understand why you have to require that namespace. I can't find an ns with that name when grepping through the project. Why did they design their API like that?

We only do static analysis so you can prevent this from happening by using that ns somehow. However, since the ns doesn't exist / doesn't contain anything that might be tricky.

Right now referencing a keyword in the ns doesn't prevent it from being pruned, but I could change that if that would be an acceptable escape hatch for you? If I do that, you could prevent the ns from being pruned by discarding a value like :joplin.dt.database/prevent-cljr-from-pruning-this-ns somewhere in the file.

Alternatively the juxt guys could actually ship an ns by that name which re-exportedidentity, or whatever, so you had a harmless symbol to reference.

//cc @martintrojer

[nblumoe]

@expez dt in the namespace needs to be replaced with the database type you are going to use. For example JDBC: https://github.com/juxt/joplin/blob/master/joplin.jdbc/src/joplin/jdbc/database.clj

In those database specific namespaces, multimethods are implemented.

[expez]

A workaround would be putting the following, somewhere in the file:

#'joplin.jdbc.database/migrate-db ; clj-refactor: don't prune implicit deps!

or alternatively, I think this should work too:

(ns example
  (:require [joplin.datomic.database :as datomic]))

(datomic/migrate-db
  {:db {:type :dt,
        :url "datomic:mem://test"}
   :migrator "joplin/migrators/datomic"
   :seed "seeds.dt/run"})

I'd argue that this code actually reads better too.

[nblumoe]

Thanks for the suggestions!

However, I am not going to use them, though. I don't want to introduce code changes just because of the tooling. The second suggestion also would require touching the code once again, if the chosen database implementation changes (unlikely, but still a negative effect).

I am not sure, if it is worth trying to catch such cases from clj-refactor. It should be rather rare and one just needs to be a little bit cautious, not to accidentally remove needed dependencies. Being able to respect such dependencies without having add anything to the code would be cool though. :)

If you think it is not worth spending time on that, of course feel free to just close the issue.

Thanks again for the help!

[expez]

The second suggestion also would require touching the code once again, if the chosen database implementation changes (unlikely, but still a negative effect).

You would have to change the code anyway, e.g. to change the :url and :migrator if the DB was changed.

Being able to respect such dependencies without having add anything to the code would be cool though. :)

Yeah, but I can't think of any way to do this short of looping over the deps and checking if the ns still evaluates if I remove it. An earlier version of clean-ns used tools.analyzer to find symbols which were in use, but much like the approach of looping over the deps and attempting removal (which I think is what slamhound does) it only works when you code is in pristine condition. So doing this would be a trade-off.

If you think it is not worth spending time on that, of course feel free to just close the issue.

It would be a ton of work, make the op less forgiving about the state of your code, and since we've found (at least to me) acceptable workarounds for what was already an edge-case, I don't think it's worth it.

[Malabarba]

FWIW, I agree with @expez that his second option reads better (and is shorter).

However, here's yet another possible workaround:

(ns example
  (:require [joplin.core :as joplin]))
(require 'joplin.dt.database)

(joplin/migrate-db
  {:db {:type :dt,
        :url "datomic:mem://test"}
   :migrator "joplin/migrators/datomic"
   :seed "seeds.dt/run"})