clojure-emacs/clj-refactor.el

Create cljr-slash variant that uses the `suggest-libspecs` middleware op

vemv opened this issue · 9 comments

vemv commented

Create a cljr-slash variant that uses the suggest-libspecs middleware (clojure-emacs/refactor-nrepl#384), and has a more streamlined UI than current cljr-slash (since it would never ask for language context).

These are the inputs it has to send to the middleware:

  • The user input, e.g. set when the user typed set/
  • The filename extension, grabbed from (buffer-file-name)
  • The entire value of cljr-magic-require-namespaces

It will receive the following response, order by priority:

["[namespace :as alias]"
 "[another-namespace :as alias]"]

All that has to be done Emacs side is letting the user choose one of those, and insert it.

No logic should be performed here; that is done refactor-nrepl side.

The UI for letting the user choose should be customizable, in an Emacs-idiomatic fashion.

The variant should be opt-in, and disabled by default.

vemv commented

@dgtized: the latest clj-refactor (3.5.6; will be available within a couple hours) / refactor-nrepl (3.5.5; released) include a draft of the new middleware, namely:

clojure-emacs/refactor-nrepl@8825e26

which you can try like this:

(thread-first "cljr-suggest-libspecs"
  (cljr--create-msg "lib-prefix" "s"
                    "language-context" "cljc"
                    "preferred-aliases" (prin1-to-string cljr-magic-require-namespaces))
  (cljr--call-middleware-sync "suggestions")
  parseedn-read-str)

as you can check out in the commit, this draft is merely focused in getting the API right (input and output format). The strings to be produced are subject to (much) improvement. For now we just return essentially the same as the old op namespace-aliases, with some basic context (language) awareness.

I hope this helps getting the ball rolling.

Started working on this, and I have a decent implementation, but went back to add tests and realized the build is broken from your last two commits. Do you mind addressing that?

vemv commented

Hi! Excited to hear that.

The build is failing for a random reason, back then I did verify that even re-triggering a build from the latest green commit now would fail.

Obviously it has to be addressed, but you can rest assured that nothing is broken.

Unfortunately the pattern used in the tests that are failing is the same one I need to write tests to verify the new behavior. So, while the build may not actually be broken, I'm not sure if I can add tests without this being fixed?

I suspect this is related to jorgenschaefer/emacs-buttercup#222. Specifically because the failing tests are ones that are asserting the return value of a form with a macro, and are failing because the value incorporates the form and not the value. Ie:

(it "returns js for js alias in cljc file."
    (expect (cljr--with-clojure-temp-file "foo.cljc"
              (insert "(ns foo)")
              (cljr--unresolved-alias-ref "js"))
            :to-equal "js"))

fails but

(describe "cljr--language-context-at-point"
  (it "returns clj in a clj file"
    (cljr--with-clojure-temp-file "foo.clj"
      (insert "(ns foo)")
      (expect (cljr--language-context-at-point)
              :to-equal '("clj" nil)))))

passes as a work-around. Anyway, I'll take a look again later.

vemv commented

Anyway, I'll take a look again later.

Alright! Will be appreciated, I don't have enough capacity to dive into Elisp stuff these days.

However I can keep refining / actually implementing the new refactor-nrepl middleware upon your feedback.

Merging a PR of yours into clj-refactor.el isn't a pre-requisite. If that saves you time, you can, for instance, open a Draft PR, even if red, and we refine both frontend and backend using that PR as a starting point.

I believe this issue was resolved by #532 and #541. I know there is still work on the nrepl side to improve the new behavior around cljr-slash, and at some point we need an issue for changing this behavior to be the default and deprecating the old approach, but I don't think there is anything else to do on the elisp side to support this feature?

vemv commented

Surely!

I'll eye it again soon.

btw, last Saturday I worked substantially on clojure-emacs/refactor-nrepl#384 and have a few new test cases passing. The plan is to encode all examples (yours and mine) as cases.

New iteration soon, when it's ready I'll cut a release and ping you for code review.

Cool, happy to review. I think the remaining issues on clojure-emacs/refactor-nrepl#384 is probably more related to #530 than this issue but happy to hold onto both if you see fit.

I suspect the bulk of the remaining changes in 384 are language-context specific which are discussed in that issue and in 530, but require mostly backend changes to support.

vemv commented

I believe this issue was resolved by #532 and #541.

Indeed, things are looking great by now.