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

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

Closed vemv closed 1 year ago

vemv commented 2 years ago

Create a cljr-slash variant that uses the suggest-libspecs middleware (https://github.com/clojure-emacs/refactor-nrepl/issues/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:

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 2 years ago

@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:

https://github.com/clojure-emacs/refactor-nrepl/commit/8825e26989726a849f0910988c764f136f85e8fc

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.

dgtized commented 2 years ago

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 2 years ago

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.

dgtized commented 2 years ago

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 https://github.com/jorgenschaefer/emacs-buttercup/issues/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 2 years ago

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.

dgtized commented 1 year ago

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 1 year ago

Surely!

I'll eye it again soon.

btw, last Saturday I worked substantially on https://github.com/clojure-emacs/refactor-nrepl/issues/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.

dgtized commented 1 year ago

Cool, happy to review. I think the remaining issues on https://github.com/clojure-emacs/refactor-nrepl/issues/384 is probably more related to https://github.com/clojure-emacs/clj-refactor.el/issues/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 1 year ago

I believe this issue was resolved by https://github.com/clojure-emacs/clj-refactor.el/pull/532 and https://github.com/clojure-emacs/clj-refactor.el/pull/541.

Indeed, things are looking great by now.