Skip to content
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

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

Closed
vemv opened this issue Aug 20, 2022 · 9 comments
Closed

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

vemv opened this issue Aug 20, 2022 · 9 comments

Comments

@vemv
Copy link
Member

vemv commented Aug 20, 2022

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
Copy link
Member Author

vemv commented Aug 21, 2022

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

@dgtized
Copy link
Contributor

dgtized commented Sep 3, 2022

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
Copy link
Member Author

vemv commented Sep 3, 2022

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
Copy link
Contributor

dgtized commented Sep 3, 2022

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
Copy link
Member Author

vemv commented Sep 3, 2022

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
Copy link
Contributor

dgtized commented May 23, 2023

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
Copy link
Member Author

vemv commented May 24, 2023

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.

@dgtized
Copy link
Contributor

dgtized commented May 24, 2023

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
Copy link
Member Author

vemv commented Jul 24, 2023

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

Indeed, things are looking great by now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants