Fork me on GitHub
#calva
<
2021-12-11
>
Tomas Brejla17:12:45

Hello. I use tab and shift+tab quite often and while they work perfectly most of the time, I still found that in some situations it breaks my code quite a bit. Here's an example of one such situation. This works fine:

(loop [{:keys [foo] :as state} {:foo 42}]
  (if (= foo 0)
    "Looped all the way to the end"
    (recur (update state :foo dec))))
.. with ^^^ I can both tab and shift+tab anywhere and the code's formatting will remain the same. But now if I choose to add a newline just before the first [ after loop, I end up with following code:
(loop
 [{:keys [foo] :as state} {:foo 42}]
  (if (= foo 0)
    "Looped all the way to the end"
    (recur (update state :foo dec))))
With this form, I can still press tab and the code remains the same (or formatting may just remove some unnecessary whitespaces here and there, if any) But when I press shift+tab , the structure of the code breaks. It this behavior a bug or feature? 🙏

pez18:12:23

Interesting that you use the shift+tab feature. I was wondering the other day of anyone did. I wonder why it can’t handle that second case… Let’s ask @UGNFXV1FA who knows a bit about how Parinfer should work.

Stefan20:12:21

I don’t know. In my vim setup shift-tab doesn’t do anything. According to @U050XCE7C in https://github.com/BetterThanTomorrow/calva/pull/1381#issuecomment-971117662 shift-tab should “dedent”, which is indeed what I would expect. That’s what it always does…

pez20:12:16

That’s not the question here, actually. In Calva shift-tab infers parens. So the question is what should happen in the case above when parens are inferred.

👍 1
pez09:12:42

@U01LFP3LA6P, can you try how the build in https://github.com/BetterThanTomorrow/calva/pull/1429 behaves for this case? Not sure if you have followed any of the parinfer things, but just let me know if you need more context.

Tomas Brejla09:12:22

@pez that build no longer seem to have Infer parenscommand assigned to shift+tab. This is 2.0.229:

Tomas Brejla09:12:35

but in that PR build I can't find such command at all

pez09:12:13

Yes, that command is removed there. It introduces a parinfer mode instead. We can probably put the command back, if it still makes sense.

Tomas Brejla10:12:40

Hmm.. I was using shift-tab relatively often to balance missing parens. Someohow I got to these sort of situations now and then. Perhaps the time has come for me to finally learn to use parinfer and therefore ideally not to have to think about missing parens at all 🙂

pez10:12:06

Maybe. Personally I almost never have that situation. It will also depend on wether we will be able to support parinfer in a meaningful way.

Stefan07:12:15

@pez Sorry I misunderstood. And indeed I didn’t know shift-tab existed, but it seems quite useful actually 😀 I think you give me too much credit about parinfer, but it seems to me that @U01LFP3LA6P’s example should work. Can’t really contribute anything beyond that I’m afraid…

emccue21:12:08

okay so for a concrete example of how i get confused with the defrecord stuff. When i put a newline here, i expect the symbol for the protocol to not merge up into the vector

bringe23:12:16

That’s odd. CC @pez .

emccue01:12:50

this is 227 with parinfer and strict mode, if that helps

pez10:12:43

Please add it as a comment to this PR: https://github.com/BetterThanTomorrow/calva/pull/1429 That said, I still can’t follow what is going on in the GIF. 😃 A text example showing the transitions and comparing expected with actual would be great.

Marc O'Morain23:12:07

@brandon.ringe @pez I've updated my PR to enable the Test Explorer. I've enabled it behind a setting, default false. There is some missing functionality: • It currently shows results in the Test Explorer UI when you run tests using the Calva commands, but it doesn't run tests when you press the “Run Tests” button from the Test Explorer UI. • I'm not sure what then idiomatic way to output failure messages it. The UI seems to expect the results all on one line. • Discovering Tests before you run them will require integration with the latest Clojure LSP, which is something that's beyond my expertise at present. But the PR might be OK to merge since the code is behind a flag? And we can iterate further on it? I'm not sure what your preference is for releases - continuous delivery behind a disabled feature flag or a single “big bang” release. I Fabius the former.

bringe23:12:01

Awesome. We’ll see what’s needed for the clojure-lsp integration once that feature is released. The failure messages, we probably should continue to print them to the output window, at least for now. I’m not sure if it already does that.

Marc O'Morain23:12:15

Yup, I've left the output window behavior unchanged. The only change in behavior is when the Test Explorer setting is enabled, I don't set the Diagnostics (which gives the little red underline on failed tests). This is because the Test Explorer makes the line red itself, so the Diagnostic is redundant. If you disable my setting, then the Diagnostic behavior remains unchanged.

bringe23:12:47

Sounds good

ericdallo23:12:12

I should merge the test-tree branch on clojure-lsp soon. Also, I implemented the Emacs client side to show the test-tree, so it should not be too different on vscode, let me know if need any help

Marc O'Morain23:12:25

It would be good to bump the version of cider-nrepl used with Jack In at some point, to the latest version, to get this fix ^

bringe23:12:30

Feel free to bump it

bringe23:12:59

Just make sure to test some basic repl functionality and read the changes to see if anything specific needs to be tested

bringe23:12:31

> it doesn’t run tests when you press the “Run Tests” button from the Test Explorer UI. It does run them for me

Marc O'Morain23:12:03

Is bumping the cider-nrepl version a risky change? At CircleCI when we make small changes like that we always end up breaking folks builds (any time we change anything in our build environment that shouldnt be visible to the customer, like the Linux kernel version or Docker Runtime)

bringe23:12:49

There’s generally low risk with cider-nrepl, unless they bumped the major version. I don’t think we’ve experienced many issues with updating it in the past.

👍 1
bringe23:12:01

And it’s a simple thing to revert

bringe23:12:15

In the in-editor test failure message there’s no space between actual and the result, not that this would be used very often for larger results.

Marc O'Morain23:12:43

The missing space is a bug from when I was iterating on the formatting. That's one of the places that I need to think carefully about. If you have any new lines in that message, they appear as a little lf character, which looks strange. And I would expect there to be lots of new lines in Clojure error messages.

pez08:12:09

Releasing behind a setting sounds great to me.

pez08:12:32

The mocca test explorer outputs only a single line inline and the rest is printed to an output channel. Iirc. We could do something like that? But first release. 😎

Marc O'Morain23:12:05

@ericdallo do you know how Calva code can read the list of tests from Clojure LSP? Does the LSP expose that as a JavaScript API, or is exposed as part of the LSP protocol? I've never done any extension<->LSP interop

ericdallo23:12:54

I created a custom LSP extension for that as the LSP protocol doesn't provide tests information but allow protocol extension

ericdallo23:12:00

Basically, clojure-lsp will produce a custom notification with data which the client(calva) can listen and build the test tree or persist the data

ericdallo23:12:58

https://github.com/emacs-lsp/lsp-mode/pull/3250 This is a example on how it works on emacs

👍 1
bringe23:12:18

Oh, I see: :notification-handlers (lsp-ht ("clojure/textDocument/testTree" #'lsp-clojure--handle-test-tree))

👍 2