Agree that it is hard to follow. I spent a long time staring at it for https://golang.org/cl/215742 -- and that CL simplified some things but made others more complicated. Also, feel free to send those reviews my way...

Can we list what we'd want to change? Off the top of my head, the following things are complex or confusing:

1. the fact that commands interact with an in-process LSP server, and therefore have to translate async LSP into a synchronous command
2. handling of the LSP connection: each subcommand connects and terminates the connection itself, duplicating logic and tightly coupling to the connection
3. the internal@ hack/optimization for cmdtests
4. special handling to default to the serve subcommand
5. sharding of flags across the application and the subcommand
6. handling of files, and translating to and from LSP file coordinates / URIs

Anything else come to mind?

Some of these probably can't/shouldn't be fixed, for example I think executing commands over LSP is The Right Thing To Do. Also, it's probably too late to stop defaulting to the serve subcommand (or is it?!).

As a starting point, I think we can improve (2) and (3): it would help to lift handling of the cmdClient / jsonrpc2.Conn out of .Run and up to the application layer, instead injecting in the connection to the subcommand. That might also simplify the internal@ optimization. I can take a stab at this.

Another straightforward improvement is just to add a bunch more documentation around the tricky parts.

WDYT?

Some of these probably can't/shouldn't be fixed, for example I think executing commands over LSP is The Right Thing To Do. Also, it's probably too late to stop defaulting to the serve subcommand (or is it?!).

Yes, I think is too late for that.

As a starting point, I think we can improve (2) and (3): it would help to lift handling of the cmdClient / jsonrpc2.Conn out of .Run and up to the application layer, instead injecting in the connection to the subcommand. That might also simplify the internal@ optimization. I can take a stab at this.

Agree with this plan - for me (3) is the hardest to reason about. Thanks!

Another straightforward improvement is just to add a bunch more documentation around the tricky parts.

Definitely. Even just writing down how the tests work would help immensely.

Another point of confusion to add to the list - we originally had a gopls query command for which the feature commands would be subcommands. Somehow this got lost along the way, but definition is still invoked as gopls query definition. We should make this consistent in one way or another. Maybe @ianthehat can shed light on what the benefits of query would have been?

Oh, another that occurred to me (perhaps that can't be fixed) is the fact that options is a func(*source.Options). That frequently confuses me -- especially since source.Options is huge.

Agree with this plan - for me (3) is the hardest to reason about. Thanks!

Do you think it's reasonable to expect that workspace initialization will ever be cacheable enough to delete the internal hack altogether? In other words, for cmdtests we initialize a gopls remote and just connect each cmd to a new session on the remote? That would be the ideal solution (and also would be an optimization for real-world usage of commands).

I think the actual command line needs some thought.

• I do not think it is too late to change it at all, but once we go beta it might be.
• We should not default to the serve command
• We should put some effort into separating automation verbs (and their flags) from user verbs. The former we should promise backwards compatibility but not worry about ugliness, the latter we should be happy to change/break for the benefit of clarity at any time.

I agree the internal hack should be vaporized, it ought to be possible (but maybe with a different cleaner hack added to the client server behavior)

Closing this issue as no longer actionable. The command package has changed a bit, and while it's still not as clear as it could be, this issue does not track work that we plan to do.