Prompt for namespaces with language context annotation

[dgtized]

Rewrites #521 using a defcustom cljr-magic-require-prompts-includes-context. This is also the followup to #522, which introduced refactors to cleanly apply this change. This is an improvement to the cljr-slash magic require functionality, where inserting a slash after an alias used elsewhere in the project will be automatically added as a namespace alias for the current file.

If the defcustom cljr-magic-require-prompts-includes-context is set to t, cljr-slash will prompt for namespace completion if there is more then one match or short-circuit if only one. Instead of first prompting for language context, it now shows all the possible contexts (:clj), (:cljs), and (:clj, cljs) the alias has been referred to elsewhere in the project along with the recommended libspec. Aliases that are shared across more then one language with the same namespace are now grouped into a single option, allowing cljr-slash to short-circuit and add the require without prompting.

If the defcustom is false, it uses the existing behavior, which if in a cljc file will first prompt for the language context, and then prompt for potential libspec entries to resolve the alias.

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!

[vemv]

Thanks much - code coverage looking great!

Hope not to bother much, but could you please refrain from using cl-* facilities? Rationale

[vemv]

Suggested change

	"Whether `cljs-slash' should include the language context in magic
	"Whether `cljr-slash' should include the language context in magic

Suggested change

	"Whether `cljs-slash' should include the language context in magic
	"Whether `cljs-slash' should include the language context in magic

[dgtized]

I can switch the cl-destructuring-bind to pcase-let or something if that helps, with regards to the cl-loop though, it's a little trickier as there doesn't seem to be a nice way to walk over the keys and values of a hash-table together and collect the results in elisp (or I'm not aware of that functionality). To be honest I mostly have avoided using hash-tables in emacs as they are often so awkard to interact with due to lack of support in core libraries. I guess I can look into walking the keys with mapcan or mapconcat somehow, or an imperative loop with multiple nested dolists appending to a single list?

Also I know the reason everybody avoided (require 'cl) is it infected core methods and left it confusing which behavior was in use, but I thought cl-macs was fine and basically considered core as everything was name spaced. I believe those are the only usages I've added. All of the cl-first, cl-incf, cl-rest, cl-second, cl-assert, etc were already in use, which is the main reason I added the missing require.

I do appreciate the maintenance concern, though to that end I hope someday we can eliminate cljr--list-namespace-aliases entirely by exposing a middleware method that returns the list directly instead of flattening out the hash-table. The longer I've dealt with different APIs the more I've realized that they should provide lists of all the items at the top level if at all possible, and allow the client to index by any keys they want instead of prescribing a particular indexing pattern on the backend. Anyway, well aware it's quite a while before that can be deprecated (if ever), but that would be my preferred approach for removing the cl-loop cases.

I'll fix the typos and switch cl-destructuring-bind to pcase-let and then wait to hear for your preferred approach on replacing the cl-loop over the hash-table.

[vemv]

Thanks much for the thoughtful response and subsequent commits.

I hope someday we can eliminate cljr--list-namespace-aliases entirely by exposing a middleware method that returns the list directly instead of flattening out the hash-table.

Could you sum up what's needed in a refactor-nrepl issue? I'll be pleased to deliver it asap.

[dgtized]

Thanks, and thank you for you time spent reviewing as well. After some further thought it's not quite so tricky as I thought with seq-mapcat, though I still maintain the cl-loop is more readable here. If anything I'm more concerned about the maintainability of the mutation in the map/reduce. Regardless it's covered by a pretty comprehensive test. Changing from cl-loop to seq-mapcat changed the order of the list but the structure is still stable.

I still think that replacing the entirety of cljr--list-namespace-aliases with a better API would be better in the long run. Returning something like [{alias: str, namespace: str, language: str}, ..] would replace the nested mapcat. Or preferably if the API returns something like [{alias: str, namespace: str, contexts: [str,...]}, ...], it would also remove the map/reduce. I'll try and submit an issue in refactor-nrepl in a bit. Either way we still need to support the existing method for a bit, so it's probably better to use the existing API a bit longer and then upgrade once it's clear what the best format and other uses cases are. I did see in middleware repo that it looks like we also have frequency information for aliases, and I suspect we could add that as an additional key for each alias/namespace mapping in the list.

Thanks again for your time.

[bbatsov]

I still think that replacing the entirety of cljr--list-namespace-aliases with a better API would be better in the long run.

And you're probably right. The likelihood of removing this breaking something is slim to none IMO and given our limited resources I wouldn't go overboard maintaining a private API that's unlikely to be used by anyone.

And I'll reiterate my opinion that replacing the cl-loop just for the sake of replacing it is probably not the best way to go here.

[vemv]

Is this still needed?

[bbatsov]

Quite unlikely. It has been cl-lib for a very long time.

[dgtized]

clj-refactor is using cl-case, cl-incf, and cl-assert all of which are not present in cl-lib but are present in the cl-macs wrapper.

[dgtized]

Should I add a note to the readme for this? It's user facing, but the cljr-slash feature doesn't appear to be documented there.

[vemv]

It could be added in https://github.com/clojure-emacs/clj-refactor.el/wiki#magic-requires

[dgtized]

Happy to document it in the wiki once this is merged, I guess I was mostly curious if there was any other documentation I should update or add in this PR.

[dgtized]

Added documentation in the wiki: https://github.com/clojure-emacs/clj-refactor.el/wiki/Home/_compare/f70058bac1388cb7094ed56d6b63f1b8dc18f021...ad3bc7a5d570053501464a320122f596f999dd57

[vemv]

From your screenshot, it looks like typing t/ will result in one of the :clj or :cljs choices being selected and inserted.

What if it inserted both, via a reader conditional? Especially if there's exactly just one choice for each of :clj, cljs.

I know it might be some extra work but I think it's the right thing to do. It would sound very intuitive and enjoyable for users.

Feel free to keep in mind this (likely) requirement when creating an issue against refactor-nrepl.

Cheers - V

[dgtized]

I agree that case would be magical and I'm game to incrementally improve in that direction. However, I want to clarify the most common use case for this is not with libraries that have a different namespace and the same alias delineated by a reader conditional. The main use case for this change is using a known alias for a cljc library in clj file when previously it's only been used in a cljs file or the first time it shows up in a clj file and it's only been used in a cljs or cljc file. Those cases do not require a reader conditional regardless if it's in a cljc, clj, or cljs file. Before this PR those cases all required first to confirm that I wanted to search cljs or clj contexts and then allowed me to select a matching namespace. The new behavior allows the user to choose whichever library is most appropriate for the context and only treats the language context as a hint.

Likewise, there are a few cases where an alias is commonly referring to a cljc library in a group of cljs files, and an entirely different library in a set of clj files but the library is not a replacement for the other. As example, I have some code where gl/ refers to [thi.ng.geom.line :as gl] and I have some code where it refers to [thi.ng.geom.gl.core :as gl]. In that example both are cljc libraries so they may both be referenced in clj and cljs contexts, but I don't want it to try insert a reader conditional for both if I just so happened to only use gl.core in a clj context, and gl.line in a cljs context previously in the project.

Now for a few cases like cljs.test and clojure.test where the reader conditional is often used it would be great to detect that and prompt the user to see if they want that case. However I don't think we can reliably detect that case until we have additional information from the backend. I'm happy to discuss how we might include and improve in that direction but I think that's out of scope for this PR.

[magnars]

There is one case where we can reliably insert a reader conditional, and that is when there is already a reader conditional for that alias in the project. I do however agree that it is out of scope for this PR.

Note that there is no need for a reader conditional in your final example. You can simply refer to clojure.test. This sugar has been available since the 1.9.198 release of ClojureScript. See the Sugar section here: https://clojurescript.org/guides/ns-forms

[dgtized]

Agreed that we can reliably insert a reader conditional if it's in the project already, but AFAIK the middleware does not currently expose enough information to reliably suggest that to the user. I believe in this case it would report that as {:clj {alias [namespace1]} :cljs {alias [namespace2]}, which is indistinguishable from a case where the same alias is used for two different namespaces. So that's certainly information worth including in a future improved middleware response.

Thank you for reminding me of the clojure.test/cljs.test sugar, I'll give that a try.

[dgtized]

I thought I would add another example where alias usage is mixed across different language contexts. I hope this helps clarify:

[vemv]

The counterpoint to working "gradually" is that I would like to avoid giving users intermediate UXs that are not representative of what we actually intend to do long-term.

Likewise we also should avoid creating commits that would represent "transient states" - those can be considered churn. Very similarly, an hypothetical excess of PRs has a cost to reviewers.

I would suggest that we operate in the following manner:

• Create issues describing what is envisioned
  • Should have been done in the first place! But no problem.
  • This is the place to be super clear and concise - keep it short :)
  • Use examples (these inputs -> these outputs).
• Request whatever is needed to the refactor-nrepl side
  • This relates to both keeping the .el simple, and the features definitive/comprehensive with no transient states.

Thanks very much again for the work so far!

-V

[vemv]

Out of #528 where I could understand @dgtized's intent better, I designed the following split of tickets:

#530
clojure-emacs/refactor-nrepl#384
#531

I believe the end result will be vastly simpler. In the end, as described in clojure-emacs/refactor-nrepl#384, namespace-aliases was a middleware op that becoming more bloated/overloaded and less faithful to its original intent with each iteration. (This PR would have accentuated that)

It's better to start again from a clean slate.