Option to autoshutdown after killing last buffer

[ilohmar]

This got messy, sorry.. rewriting a branch prevents reopening the PR, so here's a new one.

The old discussion is at #305.

[joaotavora]

This got messy, sorry.. rewriting a branch prevents reopening the PR, so here's a new one.

Didn't know about that. But I would advise against closing your own PR, even if you think it's somehow "wrong". Just let it be and you can always rewrite the commtis of the branch. For me the PR is the idea. The implementation is a separate thing.

[ilohmar]

Yeah, good advice, will heed that in the future.

[joaotavora]

In all these years, it's the first time I see an actual use for the "depth" arg to add-hook.

The PR looks reasonably clean. You've obviously investigated enough, and this is one of the trickiest, if not the trickiest, part of Eglot. Two things:

• first, a question: is it so, that currently, before your PR, we first tear down eglot-managed-mode and only then send the didClose notification for the just-killed buffer? If so, am I surprised. The natural order would be reverse, as you seem to propose in the PR. But if it isn't, I don't understand how didCloses' eglot--current-server call can succeed in these conditions. Can you explain?

• once we figure out the natural order, I think we can change switch the order of the add-hook statements since we control both locally. They're always done in tandem, there's no way to do one and not the other.

• this will need tests. and the CI system has been broken for a while and I haven't had the courage to look at it. :-) But I will :-)

[ilohmar]

In all these years, it's the first time I see an actual use for the "depth" arg to add-hook.

Same here :) Relating that to your second point, I chose the depth argument on purpose, to make the order explicit from the call itself. Otherwise one has to think "backwards" (as in "this is added later, so it is prepended to the hook, so it runs first"). If you think re-ordering is clearer, go ahead, of course.

As for your first item: Yes, that is my understanding. AFAICS, the --onoff call only clears the cache, but the server stays in the hash eglot--servers-by-project, that's why (eglot--current-server) still returns it (and until this PR, it was still running, so everything worked).

This is a great example of the intricacies that I mentioned in the old PR: it's a bit tough to follow the logic, b/c the cache and the hash lose sync unnecessarily. As I said there, stuff for another PR.

Regarding tests: if you spend time on the testing setup, a mock language server (in elisp, preferably) would be great. This way, we could test eglot against the spec (or explicit deviations), in contrast to testing it against implementations beyond eglot's control. Unfortunately, I cannot commit time to look into that soon myself. My lack of familiarity with testing in Elisp would make that much more time-consuming than the kind of PR I am doing now.

Anyway, eglot is great and I am looking forward to more hacking on it.

[joaotavora]

Same here :) Relating that to your second point, I chose the depth argument on purpose, to make the order explicit from the call itself. Otherwise one has to think "backwards" (as in "this is added later, so it is prepended to the hook, so it runs first"). If you think re-ordering is clearer, go ahead, of course.

You have a point, but we'll see. The depth argument has its subtleties too, and nobody knows it. A comment stating "this is the order because blabla" would also work, imo.

As for your first item: Yes, that is my understanding. AFAICS, the --onoff call only clears the cache, but the server stays in the hash eglot--servers-by-project, that's why (eglot--current-server) still returns it (and until this PR, it was still running, so everything worked).

Oh I missed that last part, which makes sense. So I think this PR could be split into two commits bugfix/optimization + the new feature itself.

This is a great example of the intricacies that I mentioned in the old PR: it's a bit tough to follow the logic, b/c the cache and the hash lose sync unnecessarily. As I said there, stuff for another PR.

The onoff is a bit hacky, indeed. It is mostly there to reuse code but it could be broken off. Don't see an enormous need for it, though.

Regarding tests: if you spend time on the testing setup, a mock language server (in elisp, preferably) would be great. This way, we could test eglot against the spec (or explicit deviations), in contrast to testing it against implementations beyond eglot's control. Unfortunately, I cannot commit time to look into that soon myself. My lack of familiarity with testing in Elisp would make that much more time-consuming than the kind of PR I am doing now.

This is a good idea, but still so many things happen off-spec that I don't think it would be enough to abandon server-specific tests.

Nevertheless, you'll find in eglot-tests.el there are macros that help test the shutdown and project mechanics. These should, in principle, not be the ones completley broken, though they probably rely on either the rls or pyls servers that I had installed back when I was doing more intensive eglot dev.

Anyway, eglot is great and I am looking forward to more hacking on it.

This is much appreciated!

[ilohmar]

As for your first item: Yes, that is my understanding. AFAICS, the --onoff call only clears the cache, but the server stays in the hash eglot--servers-by-project, that's why (eglot--current-server) still returns it (and until this PR, it was still running, so everything worked).

Oh I missed that last part, which makes sense. So I think this PR could be split into two commits bugfix/optimization + the new feature itself.

Sure. What would you consider the bugfix, the re-ordering in kill-buffer-hook? I would rather not touch the cache stuff itself.

This is a great example of the intricacies that I mentioned in the old PR: it's a bit tough to follow the logic, b/c the cache and the hash lose sync unnecessarily. As I said there, stuff for another PR.

The onoff is a bit hacky, indeed. It is mostly there to reuse code but it could be broken off. Don't see an enormous need for it, though.

My goal would be to get rid of it entirely (without code duplication). If I can do that (I think I know how), there'll be a PR for that soon..

[joaotavora]

What would you consider the bugfix, the re-ordering in kill-buffer-hook?

yes.

[ilohmar]

Ok, which solution do you prefer: depth or re-ordering (with comment)? If it's the same to you, I would keep it as posted.

[ilohmar]

Come to think of it, re-ordering is better, else the didClose will usually go to the start of the hook and might be separated from onoff call..

[joaotavora]

I prefer reorderning, but if you feel strongly about the -1 we can keep it, of course.

Come to think of it, re-ordering is better, else the didClose will usually go to the start of the hook and might be separated from onoff call..

The thing is there is some subtletly to the depth argument that I don't yet understand. It's not there only to express ordering, but to express layering. I would say, if we can get by without using it, better, unless we perfectly understand it.

[joaotavora]

Merged after some housekeeping. Crucially, kept the default. If you can gather the votes again from the other issue and come um with a tally, then we can make another commit to set the default value.

[ilohmar]

On Sat, Oct 05 2019 01:52 (-0700), João Távora wrote: Merged after some housekeeping. Crucially, kept the default. If you can gather the votes again from the other issue and come um with a tally, then we can make another commit to set the default value.

Awesome, thanks!