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-slash` using `suggest-libspecs` middleware op #532

Closed dgtized closed 1 year ago

dgtized commented 2 years ago

Implements https://github.com/clojure-emacs/clj-refactor.el/issues/531. Still need to add changelog/readme updates if this approach is accepted.

If the defcustom cljr-slash-uses-suggest-libspec is enabled, cljr-slash will query the new suggest-libspec middleware op for suggested namespaces.

Currently it sends over the entire cljr-magic-require-namespaces as the preferred-aliases map, that may require pre-processing at some point, but the placeholder seems correct. It also correctly identifies if code block is in a conditional language context, and provides that information to the middleware to in order to assist generating recommendations.

Finally, as it's using completing-read to select namespaces, if the user inserts their own content into the response, that will be inserted as the new libspec. This allows the user to reference a new libspec, or edit it to include refers or require-macro. In the future, it might make sense to use the universal argument to temporarily force cljr-magic-requires to use :prompt, allowing a user to insert a require of their own choice more easily.

Before submitting a PR make sure the following things have been done (and denote this by checking the relevant checkboxes):

Thanks!

dgtized commented 2 years ago

I believe I've addressed all outstanding concerns, updated external documentation and added a changelog entry as well as manually retesting it in a project. I would much prefer we merge this over letting it bitrot in review. It's not perfect but it will allow folks to start testing and working off of the new middleware so we can iterate from there. I believe the next steps beyond this PR on the cljr-refactor side are to address the reader conditional parsing in #533, and then document the new syntax for cljr-magic-require-namespaces in https://github.com/clojure-emacs/clj-refactor.el/issues/530, though I think all of the logic there is in the middleware, so it's probably better to document that after it's implemented.

I believe that API is solid enough to move forward with. I didn't encounter anything critical with it in development, but now that the logic has moved into the middleware, it's likely we won't see issues in the recommendations until we encounter them in actual use.

Let me know if there is any other critical concerns that need to be addressed prior to merge.

dgtized commented 1 year ago

Just checking back in to see what else needs to be done here before it's merged.

vemv commented 1 year ago

Thanks for the ping! Will check out today / tomorrow at most

vemv commented 1 year ago

Thanks again for this first step!

Should https://github.com/clojure-emacs/refactor-nrepl/issues/384 be implemented as planned? Anything that came to mind more recently?

dgtized commented 1 year ago

I think we can proceed with the remainder of clojure-emacs/refactor-nrepl#384 as planned. However, until #533 is implemented, we won't know the correct current language context at point. My expectation remains that we will have more tweaking required on the exact recommendations from the backend, but that won't be as visible until it's in use. I'll see if I can take a stab at #533 in the coming week or two, though so it can operate with the correct local information.

Thanks for merging this, and the review!

vemv commented 1 year ago

Nice! I'll try to advance on the middleware, do feel free to ping if you'd need to have it sooner.