384 - Add test cases

[vemv]

Closes #384

Adds/refines all the tests cases we devised in #384, making them pass, and then add some more.

...sometimes what I considered correct back then, turned out to be not exactly correct or desirable.

This is in good part because we agreed in the separate notions of "buffer-language-context"/"input-language-context" after having that discussion.

With a real implementation and tests it was easier to see how it should work.

I'd love a review from @dgtized, basically for deftest suggest-libspecs-response which is one big data table. I'm open to tweak the desired behavior in a few fronts. I'll refrain from explaining it much to avoid any biasing.

Finally, at the moment the impl is clj/cljs centric. I could add bb/cljr slightly later, particularly after this round of feedback.

Cheers - V

[dgtized]

I think this is a great step forward. I'm traveling at the moment, so I tried to jot down what I could but will have to take a closer look later this weekend. I think the major discussion is what we want to do for cljc/cljc cases where we have existing aliases for cljs or clj but it's not clear if the current context is clj or cljs. I honestly think the biggest issue there is that from language spec, we can have valid cljc files that have some functions that do not work in cljs or clj. It would be amazing if the implementations caught that, at least for the active version, but I'm not sure the best path forward to help the users of cljr-slash.

The other tricky bit is that I think we need client support to figure out how to append or replace requires with a reader conditional correctly. We might be able to bypass the client by just always replacing the new require statement with whatever cljr-slash selection is taken, but still need to support that functionality.

As an aside, I sometimes wish there was a way to send back requested file edits to emacs from a server that is basically a serialized diff or sed edit command that emacs can apply to the current buffer. It feels like that would open up a bunch of other edits to be determined outside of emacs but reflected in buffer without a revert/save cycle. Probably overkill for the use case of adjusting namespace libspecs, but feels useful in a lot of other contexts.

Anyway, thought I would give you some thoughts now and then check back in once I'm settled back at home. Let me know what you are thinking on the client support for merging reader conditionals in require statements, and cljc/cljc issues. Great to see this moving forward and it's so much easier to discuss expected behavior with so many test cases!

[dgtized]

I can't remember if we account for this in elisp, do we avoid adding a second require, or appending a new language context entry to an existing require there? Ie if there is an existing require for [clojure.java.io :as io] in a cljc file and then we reference io in a cljs context, it should update the require to be [#?(:clj [clojure.java.io :as io], :cljs [something-else as io]]], but I think it's currently going to keep the existing require as it detects it's already loading the namespace.

I think that logic has already been an issue for namespace sorting as it always expands the entire require into two sorted versions, one for cljs and one for clj. Anyway, this is a good test case, but the client may need further work to support.

[dgtized]

Separately, I wonder if we should rewrite these with some other example namespace than clojure.test as I believe clojurescript internally aliases clojure.test to map to cljs.test on it's own? I guess it's also useful if an existing project is still maintaining a distinction, but we should probably set the defaults to prefer clojure.test.

[vemv]

I think that logic has already been an issue for namespace sorting as it always expands the entire require into two sorted versions, one for cljs and one for clj. Anyway, this is a good test case, but the client may need further work to support.

This is a good point.

At the same time, if an existing file already had an :as test, then test<SLASH> should not trigger cljr-slash?

Separately, I wonder if we should rewrite these with some other example namespace than clojure.test as I believe clojurescript internally aliases clojure.test to map to cljs.test on it's own?

Yeah this was pending, clojure.test is special (though worth covering) and adding a more vanilla example will clarify more behaviors.

[dgtized]

At the same time, if an existing file already had an :as test, then test should not trigger cljr-slash?

It should prevent it from triggering if it already exists. That said, I think the client currently doesn't distinguish if the :as ns-alias is only for a cljs or clj language context. So it will need to be updated to check if the as alias is active for the language context at the point of call for cljr-slash.

I think the logic should be that if it already exists for the file or language context it should skip cljr-slash, otherwise, it should try cljr-slash on the alias and update the existing reference to either include the missing alias, or amend it to include the language context specific alias.

[vemv]

I agree on having a context-sensitive check.

But perhaps we can just always append the suggestion? One can clean-ns afterwards (probably defcustom cljr-auto-clean-ns affects that).

btw clean-ns could certainly emit nicer cljc code, could be a good follow-up task. Could be easy if we bring in zprint.

[vemv]

I now added specific clojure.test awareness, so we will neer suggest cljs.test

[dgtized]

Just a style comment, but finding the long lines here a little tricky to read, and expect a diff here if a spacing gap increases is going to be too large to be readable. I wonder if representing each case as a map and providing sane defaults might be clearer and easier to maintain in the long run?

Something like:

{:lookup ["test" "clj" "cljs"]
 :expected ["clojure.test :as test"]}

Or

{:lookup ["io" "cljc" "cljc"]
 :preferred-aliases [["set" "clojure.set"]]
 :project-libspecs '{:clj  {set [something-else]} :cljs {set [something-else]}}
 :expected ["[something-else :as set]"]}

Anyway, just a thought.

[vemv]

Yeah I get this feedback from time to time for my ares.

Normally they don't get this gnarly and I don't quite like that they don't fit on my laptop.

But it's been hugely useful to see all the data, uncluttered, allowing me to think/iterate for hours.

I could refactor it as the last step when it's all sealed.

[dgtized]

I think I'm slightly confused about project-libspecs. Is that just a set of requires from other namespaces in the project, and what file context they appear in? Comment is probably sufficient, just having a little difficulty following there.

[vemv]

project-libspecs is a mocked value simulating refactor-nrepl.ns.libspecs/namespace-aliases output. Otherwise these tests would use the output of namespace-aliases when analysing refactor-nrepl itself, which is a huge value and would be confusing to work with.

Will add a comment!

(note to self - an alternative approach is to use these mock values as a "nested select keys" instruction. The outcome would be the same, but we ensure that the value is actually produced by refactor-nrepl.ns.libspecs/namespace-aliases. See https://git.sr.ht/~jomco/select-tree)

[dgtized]

I think we should always prefer leaving a valid namespace. Is a bare as foo without a namespace a valid libspec? I would be surprised to have :clj [:as io] show up unexpectedly. I think I would prefer if we returned ["#?(:cljs [test-cljs-ns :as io])] and ignored the :clj case. It's likely going to throw an error later in use if the function is called from clj, but will work fine for cljs calls.

I think this is a use case where we should try to nudge clj-kondo to warn about missing requires for a specific language context, or report a warning during libspec insertion? Technically we could prompt a user to fill this out, but I think I would prefer if this were caught as a warning.

[vemv]

Hmm... I wouldn't mind removing this feature, it's decoupled from everything else.

I think that its value is:

• saving the user from some typing
• (almost) immediately causing a compilation error, if the user didn't fill in the gap
  • as I see it, this is a self-explanatory way of warning users.

What about doing both? Emit the partial reader conditional, and a warning.

I seems easy enough to add a warning key and (message) it Emacs side.

Ultimately we can add an option for this feature, we already have options here and there.

[dgtized]

I'd rather default to not introducing errors. This is particularly an issue for figwheel/shadow-js setups in ClojureScript, as often the entire file is re-evaluated at save (instead of eval s-expr), which makes introducing errors pretty harsh. I know for me I would find the addition of the context specific require helpful, but would be very frustrated to need to clean up the empty clj case everytime I used it.

I'm not against having this error as an option, but we are getting kinda complicated to test here so would prefer we keep it simplified for now, and maybe add the option to error later once the rest of this is settled?

[vemv]

This is particularly an issue for figwheel/shadow-js setups in ClojureScript, as often the entire file is re-evaluated at save (instead of eval s-expr), which makes introducing errors pretty harsh.

Alright! We'll use that as the decider, thanks.

[vemv]

I removed this small feature, it's not quite worthwhile maintaining it / documenting it

[dgtized]

Hmm, this one is complicated. I think offering the known clj libspec is valid, but it might also mean the function should be marked as cljs only, in which case we should keep the require cljs specific.

I think if we know the context is clj and there is existing cljs libspec, it makes sense to add a clj specific import. But if we don't know anything more than the context is cljc, maybe we should just assume the cljs reference was still the intent and not add an additional context?

I suspect this may be more of an issue with the language specification, since these language contexts are weirdly open ended, but only validated at runtime by cljs or clj.

Just for further example, I believe the following cljc file is valid cljs and clojure, but functions can only be called in cljs:

(ns foo (:require [#?(:cljs [cljs.bar :as bar])]))

#?(:cljs (defn baz [] (bar/bar)))

(defn quux [] (bar/bar))

baz is only visible in cljs so all calls are legal. quux is visible in cljs and clj, but calling quux in clj is a runtime error for missing import bar.

As per my comment on the next example, I think these edge cases would be great to add to a linter, but might not have a great suggestion from cljr-slash.

[vemv]

But if we don't know anything more than the context is cljc, maybe we should just assume the cljs reference was still the intent and not add an additional context?

In principle, my stance would be to not produce broken code whenever it's reasonably possible to do so. i.e., adding "#?(:cljs [test-cljs-ns :as io])"would break the :clj branch of whatever .cljc code triggered cljr-slash.

Users can refine their intent by typing #?(:cljs ) prior to invoking cljr-slash.

We could document this in the wiki (a wiki and/or blog post will be due for explaining/fostering all our work in this area)

Just for further example, I believe the following cljc file is valid cljs and clojure, but functions can only be called in cljs:

I verified that case by adding this row just now:

"io" "cljc" "cljs" [] '{:cljs {io [test-cljs-ns]}} ["#?(:cljs [test-cljs-ns :as io])"]

Will commit

[dgtized]

I agree with this output or/with a cljs only branch like #?(:cljs [test-cljc-ns :as io]). If referenced in clj this will fail, but the cljc file is still valid cljs.

[vemv]

Yeah... I see your point but in principle I'd favor not emitting broken code, it can be a nuisance for users - maybe users that were totally expecting [test-cljc-ns :as io] to be inserted!

Maybe the io example is a bit misleading, because "io" brings to mind platform-specific behavior. But many .cljc files are intended to be cross-platform.

Imagine we were talking about malli.core instead, wouldn't then this behavior appear to make more sense?

[dgtized]

Not relevant for now, but I wonder if the fallback ordering should involve frequency of use in the project? Like if most other cljc files used the combined version suggest that first?

[vemv]

Currently the order is mostly intentful, although I agree that sorting by real project frequency would be the most objective and predictable possible order.

Can do.

[vemv]

Will add this in a follow-up PR.

[vemv]

Thanks much for the careful insights!

I honestly think the biggest issue there is that from language spec, we can have valid cljc files that have some functions that do not work in cljs or clj.

I hadn't thought about this edge case. I would be tempted to not worry about it much. Note that the PR takes care to never require namespaces - it just checks for existing resources to see if they're 'requirable'.

This keeps things side-effect-free and decoupled from analysis complexities.

Not that it cannot be done (or that it wouldn't be a nice-to-have), but savvy users can always manage to lint against invalid .cljc files.

The other tricky bit is that I think we need client support to figure out how to append or replace requires with a reader conditional correctly. We might be able to bypass the client by just always replacing the new require statement with whatever cljr-slash selection is taken, but still need to support that functionality.

The current approach/stance is to always append, and let the clean-ns middleware op do its thing. Truth be told, its reader-conditional formatting is very idiosyncratic. We could refactor it to use zprint (with its how-to-ns variant) and call it a day.

As an aside, I sometimes wish there was a way to send back requested file edits to emacs from a server that is basically a serialized diff or sed edit command that emacs can apply to the current buffer. It feels like that would open up a bunch of other edits to be determined outside of emacs but reflected in buffer without a revert/save cycle. Probably overkill for the use case of adjusting namespace libspecs, but feels useful in a lot of other contexts.

Sounds certainly doable / a good idea for future features!

[vemv]

project-libspecs is a mocked value simulating refactor-nrepl.ns.libspecs/namespace-aliases output. Otherwise these tests would use the output of namespace-aliases when analysing refactor-nrepl itself, which is a huge value and would be confusing to work with.

Will add a comment!

(note to self - an alternative approach is to use these mock values as a "nested select keys" instruction. The outcome would be the same, but we ensure that the value is actually produced by refactor-nrepl.ns.libspecs/namespace-aliases. See https://git.sr.ht/~jomco/select-tree)

[vemv]

Yeah I get this feedback from time to time for my ares.

Normally they don't get this gnarly and I don't quite like that they don't fit on my laptop.

But it's been hugely useful to see all the data, uncluttered, allowing me to think/iterate for hours.

I could refactor it as the last step when it's all sealed.

[vemv]

I think that logic has already been an issue for namespace sorting as it always expands the entire require into two sorted versions, one for cljs and one for clj. Anyway, this is a good test case, but the client may need further work to support.

This is a good point.

At the same time, if an existing file already had an :as test, then test<SLASH> should not trigger cljr-slash?

Separately, I wonder if we should rewrite these with some other example namespace than clojure.test as I believe clojurescript internally aliases clojure.test to map to cljs.test on it's own?

Yeah this was pending, clojure.test is special (though worth covering) and adding a more vanilla example will clarify more behaviors.

[vemv]

But if we don't know anything more than the context is cljc, maybe we should just assume the cljs reference was still the intent and not add an additional context?

In principle, my stance would be to not produce broken code whenever it's reasonably possible to do so. i.e., adding "#?(:cljs [test-cljs-ns :as io])"would break the :clj branch of whatever .cljc code triggered cljr-slash.

Users can refine their intent by typing #?(:cljs ) prior to invoking cljr-slash.

We could document this in the wiki (a wiki and/or blog post will be due for explaining/fostering all our work in this area)

Just for further example, I believe the following cljc file is valid cljs and clojure, but functions can only be called in cljs:

I verified that case by adding this row just now:

"io" "cljc" "cljs" [] '{:cljs {io [test-cljs-ns]}} ["#?(:cljs [test-cljs-ns :as io])"]

Will commit

[vemv]

Hmm... I wouldn't mind removing this feature, it's decoupled from everything else.

I think that its value is:

• saving the user from some typing
• (almost) immediately causing a compilation error, if the user didn't fill in the gap
  • as I see it, this is a self-explanatory way of warning users.

What about doing both? Emit the partial reader conditional, and a warning.

I seems easy enough to add a warning key and (message) it Emacs side.

Ultimately we can add an option for this feature, we already have options here and there.

[vemv]

Yeah... I see your point but in principle I'd favor not emitting broken code, it can be a nuisance for users - maybe users that were totally expecting [test-cljc-ns :as io] to be inserted!

Maybe the io example is a bit misleading, because "io" brings to mind platform-specific behavior. But many .cljc files are intended to be cross-platform.

Imagine we were talking about malli.core instead, wouldn't then this behavior appear to make more sense?

[vemv]

Currently the order is mostly intentful, although I agree that sorting by real project frequency would be the most objective and predictable possible order.

Can do.

[vemv]

Thanks for this round!

LMK if there's anything else, else I'll go ahead and fix these + cut a release.

[vemv]

I agree on having a context-sensitive check.

But perhaps we can just always append the suggestion? One can clean-ns afterwards (probably defcustom cljr-auto-clean-ns affects that).

btw clean-ns could certainly emit nicer cljc code, could be a good follow-up task. Could be easy if we bring in zprint.

[vemv]

This is particularly an issue for figwheel/shadow-js setups in ClojureScript, as often the entire file is re-evaluated at save (instead of eval s-expr), which makes introducing errors pretty harsh.

Alright! We'll use that as the decider, thanks.

[dgtized]

After updating, I'm now getting errors on every cljr-slash completion in ClojureScript that calls out to the middleware. Specifically a cljr--maybe-rethrow-error: Don't know how to create ISeq from: java.lang.Character. Unfortunately, I don't have any other stacktrace other than that. Not sure if that's a quick fix or if we should rollback?

[vemv]

Whoops. Should be a quick fix.

…

On Wed, Jul 5, 2023 at 7:05 PM Charles Comstock ***@***.***> wrote: After updating, I'm now getting errors on every cljr-slash completion in ClojureScript that calls out to the middleware. Specifically a cljr--maybe-rethrow-error: Don't know how to create ISeq from: java.lang.Character. Unfortunately, I don't have any other stacktrace other than that. Not sure if that's a quick fix or if we should rollback? — Reply to this email directly, view it on GitHub <#392 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAI354X37PPYOCLC53AFUQDXOWNEZANCNFSM6AAAAAAZNXOONM> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[vemv]

@dgtized does that only happen on cljr-slash-uses-suggest-libspec t?

(I can try it myself later - just assessing the urgency)

[dgtized]

Only with cljr-slash-uses-suggest-libspec t, but with the flag on it's unusable. I can force it back to a working version by setting cljr-injected-middleware-version to 3.6.0, and disabling cljr-suppress-middleware-warnings?

[vemv]

Ok. Sorry for the nuisance. I'll fix this as the first thing of my day tomorrow.

[vemv]

@dgtized I've cut a 3.7.1 release for clj-refactor and refactor-nrepl. Both will be available in a bit.

This is the fix:

3bda039

I'd recommend that you set -Drefactor-nrepl.internal.pst=true for getting stacktraces, should any new bug arise.

(I'll get back to the stacktrace situation later - that is a patch)

[dgtized]

Ok -- that looks like it's working better, and it did correctly insert the joint require of

#?(:clj [clojure.data.priority-map :as priority]
     :cljs [tailrecursion.priority-map :as priority])
;; => priority/

#?(:clj [clojure.data.priority-map :as priority])
;; => #?(:clj (priority/

#?(:cljs [tailrecursion.priority-map :as priority])
;; => #?(:cljs (priority/

However as expected, making an explicit reference to priority/ in a cljs context when a clj require was already in the lib-spec, or in a clj context with an existing cljs require did not update the corresponding require to include both as the client assumes the lib-spec is satisfied. So will still need to address that and figure out how to munge it together. That said this already a significant improvement even if it needs to be manually edited for any further requires.

Thanks!

[vemv]

Ok -- that looks like it's working better, and it did correctly insert the joint require of [...]

Nice QAing!

Overall I'm very happy we've made it together. This has been an intrincate, useful feature that most likely will make it into cider.el at some point.

However as expected, making an explicit reference to priority/ in a cljs context when a clj require was already in the lib-spec, or in a clj context with an existing cljs require did not update the corresponding require to include both as the client assumes the lib-spec is satisfied.

Yep. As mentioned I'd find it ok to simply append, and then let defcustom cljr-auto-clean-ns do its job (leaving aside the fact that clean-ns is pretty ugly for .cljc, currently).