Prompt user for new requires across language contexts using a single completing read

[dgtized]

Changes behavior for cljr--slash to prompt from a list of candidate requires, while annotating language contexts those requires come from. Previously in cljc files, typing short-name/ would prompt if completion should occur from cljs or clj context and then prompt a second time for an appropriate namespace to require and alias. This shortcuts the context prompt by showing the namespaces for that alias that are used in the project, and annotating whether the source is from a clj or cljs file.

I believe this solution is preferable to current behavior as often namespaces in cljc files overlap aliases used in both cljs and clj files, yet still defers to the user to determine if the require is appropriate for the current language context. I think this can still be improved when in a #(:cljs) context block as we might be able to prefer candidates with the matching context, but it's much nicer than bprompting for context on every / require in a cljc file. However I think some of the context block logic under the previous approach may have been better at detecting the only available require, so that may be a regression. I feel as if the overall workflow allows for more flexibility though if we treat it as:

1. Check if / preconditions should attempt namespace completion
2. Generate a list of candidate namespaces annotated with known aliases and language contexts (not filtered by context)
3. Filter the candidate list based on the name before the / (and possibly context).
4. Prompt the user to select a namespace from the remaining set
   a. Shortcutting if only one namespace matches regardless if it is for cljs, clj, or both
5. Add missing namespace to require

I brought this up in the clojure Slack a couple months back but forgot to add a corresponding issue. I'm happy to add an issue now if that would be the preferable workflow. For now I'm opening this as a draft PR to solicit review. I'm also happy to cleanup the commit history a bit, but thought it would give some context on how I arrived where I did. Finally, I'm having a lot of trouble updating the ecukes/espud tests that interact with the completing-read as they are only asserting the final state, so it's unclear what's failing in between. Any assistance there would be appreciated.

Thanks for your consideration, hope others find this useful!

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

• The commits are consistent with our contribution guidelines
• You've added tests (if possible) to cover your change(s)
• The new code is not generating byte compile warnings (run cask exec emacs -batch -Q -L . -eval "(progn (setq byte-compile-error-on-warn t) (batch-byte-compile))" clj-refactor.el)
• All tests are passing (run ./run-tests.sh)
• You've updated the changelog (if adding/changing user-visible functionality)
• You've updated the readme (if adding/changing user-visible functionality)

Thanks!

[expez]

Thanks for taking an interest! Most welcome to get some more eyes on the clj{c,s} features!

This definitely looks like an improvement to me 👍

I'm also happy to cleanup the commit history a bit, but thought it would give some context on how I arrived where I did.

Yeah, this will probably get merged as a single feature commit, but we can wait with that until the very end.

I'm not sure what policy we have on using cl in the clojure-emacs projects, maybe @bbatsov could chime in on that, but I believe that the use of cl-loop in particular makes it more difficult for others to quickly jump into the code base and contribute. I'd rather have a slightly less elegant / verbose solution and a lower barrier of entry 🙂

[bbatsov]

I'm fine with using cl-lib features in general, and I even liked loop - probably because Common Lisp was my first real exposure to Lisp. We can always replace this with something else, but I don't think it's a big deal.

These days there's also a built-in destructuring form as a potential alternative for cl-destructuring-bind - https://www.gnu.org/software/emacs/manual/html_node/elisp/Destructuring-with-pcase-Patterns.html

[vemv]

Will be reviewing 👍 the idea seems nice to me as the current UX is a bit redundant, yes.

As a quick note on commits, as hinted, they are now far too many. But sometimes just one squashed commit leaves a poor/conflated history.

If you have it at hand, as the last step after review you might want to leave it at a few commits, e.g. one for refactoring, another for the addition of the feature.

Personally I've never used cl-* code much over 10y of elisping. I'd much prefer if we used bare elisp, or perhaps simple libraries like seq which also happens to be very clojurey (I'd argue that clojure.core/map is more clojurey than clojure.core/for).

It matters to me because much of my maintenance around clj-refactor/refactor-nrepl is for cljr-slash. Probably many people using clj-refactor use it mostly for cljr-slash as well, and as @expez says it's good that anyone can understand what's going on.

[vemv]

@dgtized could you please annotate the GH diff with comments?

I'd like to understand what changed (especially for the changes - the additions are self-explanatory) and why.

At the same time, sometimes the best change is no change, and use additions instead.

For instance this kind of diff seems bound to give problems:

-           (list short (list long))))))))
+           (list (list (intern short) (intern long) '()))))))))

You can instead create more functions, and choose which function is being invoked. And unit-test those new functions.

So as hinted over Slack, I'd prefer a defcustom and no visible changes for now. We could promote the feature to the 'default' after a couple months, when everything is polished and time-proven.

[vemv]

Please don't change names. Even if internal, they can easily have well-known meanings for maintainers.

[dgtized]

I changed back, though I suspect will need to instead duplicate and leave the existing cljr--get-aliases-from-middleware, and add the new behavior into a new function, as I think the custom to enable/disable this behavior will need to change the dispatch from inside cljr--slash.

[dgtized]

This is ensuring that both the candidates path above and the path where we construct candidates from cljr-magic-require-namespaces both return a list of triplets, where each triplet contains the alias, namespace, and the list of contexts it appeared in.

[dgtized]

They are interned because the output from cljr--get-aliases-from-middleware are all symbols due to being a filter on the parseedn-read-str output. It's possible this could be cleaner if we change the type in cljr--get-aliases-from-middleware.

[dgtized]

So it seems like the major concern is just that we want to support the existing behavior with a defcustom flag. It's kind of an invasive change as it's adjusting the whole workflow through cljr--slash, but I think I can see how we can split there. I'm going to think a little bit though.

I wonder if all the concerns with the refactors could be addressed by splitting this into two PRs. One that simply re-arranges and adds tests without changing the existing behavior but leaves an easier cut-point to insert the new behavior, and then a second after that supports the new behavior behind a defcustom? That might make it easier to review and a safer path?

[dgtized]

Rewritten cleanly in #525 and #522.