Conversation
|
|
dhruvmanila
left a comment
There was a problem hiding this comment.
Looks good! I haven't looked very closely at the code but it all looks almost same as the one in ty_server so I've provided a couple of recommendations to de-duplicate if possible. This doesn't need to happen in this PR and we can also open a "help-wanted" issue for this.
There was a problem hiding this comment.
This looks exactly the same as the one in the ty_server, can we re-use it?
There was a problem hiding this comment.
This also looks the same as the one in ty_server except for the retry method, maybe this could be re-used as well?
There was a problem hiding this comment.
Yes, it's almost the same with the exception that it doesn't have a retry function.
It's not a "difficult" problem but there is no good place for the files that are shared between ruff and the ty server. And Client depends on Session, which makes the reuse a bit of a challenge.
Summary
This PR adds support for the LSP's cancellation requests.
This is mainly a backport of the following two ty PRs:
Unlike ty, this PR doesn't add support for retries. Retries aren't necessary because
Ruff doesn't have to handle Salsa cancellations
Requester,ResponderandNotifierand unifies them under a singleClientAPI. I found them more confusing than helpful, and I don't see any reason why we should restrict background tasks from sending requests.ClientSender: This is mostly a fallout of the above.IOThreadsout ofConnection: This removes the entire complexity around weak references because scoping rules now ensure thatConnection(and its senders) are dropped before we join the thread.MESSENGERand instead require callers to explicitly pass aClient. This helped findshow_messagecall sites whereMESSENGERwasn't initialized. It also removes global state, which is always goodSessionthat background tasks don't have. This approach is the same as r-a's.Test Plan
Started VS code with the new ruff server.