384 - Add test cases

[vemv]

Closes https://github.com/clojure-emacs/refactor-nrepl/issues/384

Adds/refines all the tests cases we devised in https://github.com/clojure-emacs/refactor-nrepl/issues/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]

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 https://github.com/clojure-emacs/refactor-nrepl/pull/392#issuecomment-1622157958, 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:

https://github.com/clojure-emacs/refactor-nrepl/commit/3bda039cfb560206b7b225dc56958bc679a9ced1

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).