Closed perrinjerome closed 1 year ago
To prevent confusion, this is about cancellation, but a completely different one from the Cancel notification for unknown message id ...
issue that was discussed in https://github.com/openlawlibrary/pygls/issues/240#issuecomment-1174672020 .
It looks really good to me. Though I'm not very familiar with the area. Do you think it warrants anding to the changelog? Anyway, from my perspective I'm happy to merge it. Would you like someone with more experience of the area to review as well?
Thanks @tombh
Do you think it warrants anding to the changelog?
yes, we should probably add a line in Added section, with something like that:
Support cancel notification for work done progress tokens
Would you like someone with more experience of the area to review as well?
I believe the part that is most important to discuss is the added API, the is_cancelled
and on_cancel
methods from https://github.com/openlawlibrary/pygls/blob/db662958c24ff6a18b9e4cef7a4a914696d4ac5f/pygls/progress.py#L77-L87
This adds two methods for more or less the same thing ... that does not smell so good. The LSP architecture made it easy to have a on_cancel
callback, but having a is_cancelled
sync method seemed to be often easier to use (as we can see in the json example).
Another idea I just have is that we could expose a cancelled
future that would be resolved once a cancel notification from the client is received, then from user code it would be possible to use if ls.progress.cancelled.done()
or ls.progress.cancelled.add_done_callback(callback)
, which is functionally equivalent to the current version of this code, but without reinventing the wheel.
I'm going to make this amendment and add a changelog entry first (not sure I will have the time to do this before going on vacation)
Ok, sounds good.
@tombh thanks, I changed API to expose futures directly, as discussed. While doing this I realised that progress.create
and progress.create_async
methods were not tested, so I added test for them ( and hit same problem as https://github.com/openlawlibrary/pygls/pull/201 - for now I added a quick workaround in the test ). I also added a changelog entry.
@perrinjerome The API looks great. Thanks for working on this.
Just a friendly ping to see where we are with this?
This looks good to merge then! Just the Pyodide tests failing for unknown reason 🤔 Even though I see you've ignored set the new tests to ignore Pyodide, hmm.
Hi @tombh thanks for the reminder, for me everything was "good enough to merge" at the time, but there was this discussion above regarding the XXX comment.
Today I rebased and I have pushed the new commits in this branch and I believe this is OK. The rebase was straightforward, the conflicts were mostly with imports being changed and slightly different names. Also, what was problematic with the previous version of this pull request has been solved by lsprotocol and pygls 1.0:
WorkDoneProgressOptions.workDoneProgress
; this was also fixed in lsprotocol and https://github.com/openlawlibrary/pygls/commit/688b36b2ae9bf57cbcea5461e17ca1ed1e59e5ab so I dropped this change.That's great, thank you.
Thanks !
About the failed pyiodide tests everything looks OK on CI, so in my understanding everything is OK now. I used to have an old appveyor connected to my repository and this was failing, so maybe that was the problem. I removed it and now it's green. In any case, if these changes cause issues, let me know and I'll take a look.
Sure, no problem.
Description (e.g. "Related to ...", etc.)
Work done progress ( relevant specifications for reference ) had a few problems. It was possible for server to create a cancellable progress, but cancellation message were not handled by pygls framework. There was an implementation of
cancel
, but it was incorrect, as pointed out in #233 , it was implemented as if cancel was initiated by server, but cancel actually initiated by client. This removes the oldcancel
method and changes thetokens
mapping of progress instance so that it holds future corresponding for each tokens. The future are cancelled when a work done progress cancellation notification is received. This way, server code can either check thecancelled
method of the future, as it is done in the json example:https://github.com/openlawlibrary/pygls/blob/130f52a938211dde90b5edd9cfc952987eec6d13/examples/json-extension/server/server.py#L208-L211
The example json extension has been updated, these changes can be tested by:
start the progress
commandhttps://user-images.githubusercontent.com/521510/178146492-b0db02f0-649a-442e-b597-d2c6fac4a4ad.mov
This also contain some related fixes - that I can submit separately if you think it's better:
WorkDoneProgressOptions.workDoneProgress
type was incorrect (I don't know how to test, vscode does not support this at this time)Everything should be detailed in individual commit messages.
Fixes #233
Code review checklist (for code reviewer to complete)