org.clojure/tools.namespace update

[dotemacs]

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~ There is no CONTRIBUTING.md ~- [ ] You've added tests (if possible) to cover your change(s)~ I've fixed the tests 😄

• [x] 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)
• [x] You've updated the changelog (if adding/changing user-visible functionality) ~-[ ] You've updated the readme (if adding/changing user-visible functionality)~

Thanks!

[dotemacs]

Please share your analysis of why updating t.n alters behavior in refactor-nrepl.

I put this terse explanation in the commit message:

With the upgrade of org.clojure/tools.namespace some of the tests, failed. This was because cljs files referenced clj namespaces. Once this was amended, all the tests passed.

But to be explicit, the tests that was failing was:

lein test :only refactor-nrepl.rename-file-or-dir-test/returns-list-of-affected-files

It kept returning these files, along with the correct .clj files:

refactor-nrepl/testproject/src/com/move/dependent_ns1_cljs.cljs
refactor-nrepl/testproject/src/com/move/dependent_ns2_cljs.cljs
refactor-nrepl/testproject/src/com/move/subdir/dependent_ns_3_cljs.cljs

The reason for that is that those files, despite being in ClojureScript, kept referencing Clojure namespace:

(:require-macros [com.move.ns-to-be-moved :refer [macro-to-be-moved]])

Once that was changed to reference only ClojureScript namespaces:

(:require-macros [com.move.ns-to-be-moved-cljs :refer [macro-to-be-moved]])

the tests passed.

I would say that in the version of tools.namespace version 1.1.0, the rules were relaxed. After that release, the library became a bit more pedantic and the fact that the tests were failing was correct.

[vemv]

Thanks much! Excellent.

Just to make sure, do the tests reflect that :require-macros stuff gets renamed?

In specific terms, I don't like this existing code:

      (is (= 4 (count res))
          (pr-str res)))));; currently not tracking :require-macros!!

• The 4 is a magic number
  • we can leave it, but ideally we'd have a couple of more meaningful assertions
• I'm not sure of what ;; currently not tracking :require-macros!! means
  • Let's address things so that we can rm this comment

Cheers - V

[dotemacs]

Pedantic "deep dive" follows.

Looking at e562e3c47fd01a9d558af02e057afffe990cc2a8 from 2015, rename-file-or-dir was added along with tests that could handle the moving of cljs files.

In commit 29ec349cbe8118b6421d22a141433a90c485aa67 this commit comment was added:

caveat: as expected cljs require-macros to clj not tracked

With this dependency:

[org.clojure/tools.namespace "0.3.0-alpha1"]

updated to:

[org.clojure/tools.namespace "0.3.0-alpha3"]

When that commit was added, it included that comment in the test file:

;; currently not tracking :require-macros!!

What does this :require-macros in cljs files refer to?

The answer lies in this commit:

e35af1d1b972cfecbf5e857384234ac3f683652d

which adds a link to this issue:

https://clojure.atlassian.net/projects/TNS/issues/TNS-51

Looking at that ticket, the issue was that in cljs files you could require NPMs like you would require Clojure namespaces, which clojure.tools/namespace didn't handle.

But a patch was submitted and merged in 1.2.0 release (as you can see from the ticket). You can see the patch better here:

https://github.com/clojure/tools.namespace/commit/f40c9e58278acb152c7b8c1c21a6b10795a99e8a

The issue we started to see is because the project was on clojure.tools/namespace at version 1.1.0. So by going to 1.2.0 we started to see failing tests, because now clojure.tools/namespace handled cljs files. So it picked up cljs files which referenced macros from Clojure files.

My argument is that with the existing changes, we're simply upgrading clojure.tools/namespace version and keeping the tests passing.

If we were to add any more enhancements, I'd do the following:

• write a test that handles renaming/moving of a cljs file that references clj macro
• instead of using numbers, like in this instance 4, I'd use something that counts the number of relevant files that needs to be touched, so that it's obvious to anybody touching the tests in future. Nothing revolutionary, but hopefully more transparent.
• clean up refactor-nrepl.ns.tracker/safe-for-clojure-tools-namespace? and refactor-nrepl.ns.tracker/default-file-filter-predicate

But I would do the above changes in the subsequent PR because this current PR is small and easy to revert. The second PR one would be more of an enhancement, rather than just a simple upgrade of a stale/old dependency.

[vemv]

Thanks for the analysis! Nice exercise in archaeology there 🔎

But I would do the above changes in the subsequent PR because this current PR is small and easy to revert. The second PR one would be more of an enhancement, rather than just a simple upgrade of a stale/old dependency.

I respect this sensibility, however for the pace of this project, it's better to atomically merge all related changes at once.

We couldn't, for instance, upgrade now and risk to be left waiting for the tests for an unknown amount of time.

Cheers - V

[dotemacs]

The last commit will break the build, but read the commit message, as I'm seeking feedback if this is a desirable direction to take the tests in, thanks

https://github.com/clojure-emacs/refactor-nrepl/pull/394/commits/f42bb7aea192db3140605c79c314d591308c1249

[dotemacs]

All the tests passed, self high five!

Let me know what you think, thanks

[vemv]

I added minor tweaks to the tests, directly for agility's sake.

Thanks for the perseverance!

[vemv]

...I'll amend that commit to change attribution