-
Notifications
You must be signed in to change notification settings - Fork 111
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
cljr--insert-in-ns does not distinguish :require and :require-macros (via cljr-slash) #512
Comments
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 |
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! |
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). |
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! |
Great (and very useful) to hear!
No issue. Thanks again for spotting the issue and suggesting a handy regex! |
Expected behavior
In a clojurescript file with a
:require-macros
form before the:require
inside thens
form, usingcljr-slash
, with sayfoo/...
, should insert the require libspec forfoo
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 thens
, with the former first. Do afoo/...
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 thens
form for the sexp matching "(:require" which matches both the:require
and:require-macros
forms. Note thatcljr--search-forward-within-sexp
just searches for an exact string. One simple fix would be to changesearch-forward
tore-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
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
The text was updated successfully, but these errors were encountered: