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--reader-conditional-context` identifies current language #541

Closed dgtized closed 1 year ago

dgtized commented 1 year ago

Addresses problems identified in https://github.com/clojure-emacs/clj-refactor.el/issues/533.

Previously, cljr--goto-reader-conditional andcljr--point-in-reader-conditional-branch-p appeared to have problems with infinite loops as they looked for specific language values in the reader conditional.

cljr--reader-conditional-context starts at the beginning of the closest enclosing reader-conditional, and walks through each language, context pair returning the language name if the point is somewhere between the start of the language keyword or the next language pair. It will also bail and return nil if it searches beyond the scope of the reader-conditional or there is no language symbol it can identify.

cljr--language-context-at-point has been amended to use the cljr--reader-conditional-context to return both the language of the file & the current context to search from.

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

Thanks!

Note that this does not add any byte compile warnings, but current release does not pass because of:

$ cask exec emacs -batch -Q -L . -eval "(progn (setq byte-compile-error-on-warn t) (batch-byte-compile))" clj-refactor.el

In toplevel form:
clj-refactor.el:130:2: Error: custom-declare-variable `cljr-insert-newline-after-require' docstring wider than 80 characters

I haven't updated the readme or changelog, though I am happy to do so, but I don't think this will result in any user visible changes until the refactor-nrepl backend is updated to make use of the context information.

vemv commented 1 year ago

Cheers. Please LMK if you'd like a release.

Should https://github.com/clojure-emacs/clj-refactor.el/issues/533 be closed / trimmed down?

dgtized commented 1 year ago

I think it would be good to do a cleanup PR that deprecates and removes any of the existing language context path to use the new approach here. I would rather just attach that to #533 to close it down, but also happy to make a new issue.

Separately though, I think we should start thinking about when to enable the cljr-slash-uses-suggest-libspec by default and start the process of deprecating the old way. I think almost everything for that path is done on the emacs side, we can definitely continue to improve in the middleware, but it would be useful to start nudging users in that direction. I think the two big issues remaining from the original scope were both to add language context support to the middleware, and to get the middleware to parse the user configured namespaces that incorporates language context information. Both of which are additional features ontop of what we had before the feature flag was introduced. Anyway, let me know if we should start that process.

vemv commented 1 year ago

I would rather just attach that to https://github.com/clojure-emacs/clj-refactor.el/issues/533 to close it down, but also happy to make a new issue.

Alright! Let's do it.

Separately though, I think we should start thinking about when to enable the cljr-slash-uses-suggest-libspec by default and start the process of deprecating the old way

The answer is easy for that one - when the new middleware becomes high-quality / comprehensive. I should roll up my sleeves. Probably allocating about a pomodoro a day we could have it reasonably soon.