Handle progress messages #36
Conversation
|
This looks good. I previously looked to using the progress API but it wasn't supported by CoC at the time. An abstraction sounds like a good idea. Happy to merge this API change first. Build is failing however.
When the session is initialized the |
Just a VO to represent a token and hide the detail of its generation. It will be easier to maintain in time if there is changes in the spec.
camilledejoye
force-pushed
the
handle-progress-messages
branch
from
39934d6 to
d0a5c52
Compare
I had to upgrade php-cs-fixer to the latest version. There is a dedicated commit for it.
👍
Any preference on how to make it available: standalone service or through the client API ? I will start with that for now, I think it's cleaner to reuse this facade. |
Standalone service, it's basically a helper class that this package can provide and can be made available in the language-server extension. Can add a facade later (if one doesn't already exist) but would rather avoid contaminating the ClientAPI... |
camilledejoye
force-pushed
the
handle-progress-messages
branch
2 times, most recently
from
311e582 to
2f6cd2f
Compare
camilledejoye
force-pushed
the
handle-progress-messages
branch
from
2f6cd2f to
2185107
Compare
| "amphp/phpunit-util": "^1.3", | ||
| "ergebnis/composer-normalize": "^2.0", | ||
| "friendsofphp/php-cs-fixer": "^2.17", | ||
| "friendsofphp/php-cs-fixer": "^3.0", |
There was a problem hiding this comment.
not related: should really update this on all Phpactor projects
| $this->notifier = new WorkDoneProgressNotifier($api); | ||
| } else { | ||
| $this->notifier = new MessageProgressNotifier($api); | ||
| } |
There was a problem hiding this comment.
not blocker: would remove else here and delegate to createNotifier() with an early return? (mostly because else is avoided just about everywhere else)
| $this->assertEquals('$/progress', $message->method); | ||
| $this->assertEquals((string) $token, $message->params['token']); | ||
| $this->assertEquals('end', $message->params['value']['kind']); | ||
| $this->assertEquals('end message', $message->params['value']['message']); |
There was a problem hiding this comment.
for consistency (AFAIK): replace $this-> with self::
|
Really nice work 👍 I'll make the suggested changes |
|
Made the changes in |
2 participants
Hi there,
It's been a long time since have participated, I finally managed to free some of it so I'll try to catch up 😄
Switching to Neovim builtin LSP I realized that we don't handle the progress notification.
This PR provide a first basic implementation and I would have liked your feedback opinion on different points:
Server initiated progress
I had the idea of adding a
WorkDoneProgressTrackerto follow the different progresses and be able to throw errors if we try to reuse a token but I wonder if it's not a bit "much".In addition it forces us to keep track of the different tokens and their state which might at some point begin to be a memory issue during long sessions so we will also have to deal with the cleanup of old tokens...
I wonder if it would be interesting to provide a helper class for progress notification, which goal would be to either use the
$/progressouwindow/showMessagedepending on the client capabilities.This way we can keep sending progress to the client even if it does not handle
$/progressand we don't have to duplicate the logic every where.If so, how could I retrieve the client capabilities ?
I remember back in the days we talked about some kind of "capabilities provider" that we could inject but I think we ended up injecting the capabilities required by a service directly instead ?
How would you see this helper, as a standalone service with a dependency on the
ClientApiand capability or as a "custom" client inside theClientApiin which we inject the capability and gave the responsibility to instantiate the helper ?