search

clojure-emacs / clj-refactor.el

A CIDER extension that provides powerful commands for refactoring Clojure code.
GNU General Public License v3.0
771 stars 111 forks source link

cljr--insert-in-ns does not distinguish :require and :require-macros (via cljr-slash) #512

Closed genovese closed 2 years ago

genovese commented 2 years ago

Expected behavior

In a clojurescript file with a :require-macros form before the :require inside the ns form, using cljr-slash, with say foo/..., should insert the require libspec for foo in the :require form.

Actual behavior

Instead, it inserts it in the :require-macro form, which is not what is desired (or even correct in general).

Steps to reproduce the problem

Create a clojurescript file with :require-macros and :require forms in the ns, with the former first. Do a foo/... in the file where foo is an alias in the project. The libspec will be put in :require-macros.

The reason this occurs is that the function cljr--insert-in-ns which receives ":require" as the type searches from the beginning of the ns form for the sexp matching "(:require" which matches both the :require and :require-macros forms. Note that cljr--search-forward-within-sexp just searches for an exact string. One simple fix would be to change search-forward to re-search-forward and add a suffix to the pattern that ensures the matched string is not part of a longer "word" (e.g., \(?:$\|[^-a-zA-Z]\). (But of course there are other approaches and that may not be desirable for other reasons....)

Environment & Version information

Emacs 28.0.60 running on Mac OS X 10.14.5.

clj-refactor.el version information

clj-refactor version 3.1.0 (package 20211110.1203). I don't have time right now to update this, but I took a look at the current code, and the cljr--insert-in-ns appears to be the same. Nor could I find any other issues addressing this problem.

CIDER version information

;; CIDER 1.2.0snapshot (package: 20211108.621), nREPL 0.9.0-beta4
;; Clojure 1.10.3, Java 15.0.1

Leiningen or Boot version

Using deps.edn and shadow-cljs 2.16.12

Emacs version

28.0.60

Operating system

Mac OS X 10.14.5

vemv commented 2 years ago

Thanks much for the detailed report!

Let us know if you can contribute a PR with the suggested fix (ideally accompanied by a unit test, which is a relatively new thing in the codebase - see the tests/unit-test.el file)

genovese commented 2 years ago

I'll try to do that soon when I get a chance. I wrote a quick fix for myself and can try to incorporate that. Thanks!

vemv commented 2 years ago

Thanks to you!

And yes, a fix that's been personally tested is twice as good :)

btw, how's the performance of cljr-slash for cljs projects? Historically it had been slower than that of JVM clj, but we believe it's improved since the 3.x.x series.

I never got to personally check that (as I don't hack on cljs as much these days).

vemv commented 2 years ago

https://github.com/clojure-emacs/clj-refactor.el/blob/2372588844cbfa7b10ab534d3975bb20a4c36b8b/CHANGELOG.md#340

genovese commented 2 years ago

Sorry for the delayed response; I've been crushed under things the past week or two.

First, I've found the performance really good. In fact, I did not at first realize what was causing the inserted lib-specs; it was that seamless. So, terrific!

Second, it looks like you have fixed the problem already. Apologies for not getting the PR to you sooner; it was on my list but I'm a bit behind...

Thanks!

vemv commented 2 years ago

First, I've found the performance really good. In fact, I did not at first realize what was causing the inserted lib-specs; it was that seamless. So, terrific!

Great (and very useful) to hear!

Second, it looks like you have fixed the problem already. Apologies for not getting the PR to you sooner; it was on my list but I'm a bit behind...

No issue. Thanks again for spotting the issue and suggesting a handy regex!