Dynamically register/unregister client capability #67
Conversation
danixeee
force-pushed
the
danixeee/update-lsp-features-and-types
branch
from
0c611e9 to
bce7e48
Compare
There was a problem hiding this comment.
As far as I can tell, this looks good. Please take a look at the few comments.
Also, this is written in the description of the issue "This PR also updates features.py and types.py to support the current LSP specification." This seems like it should be a part of the PR's description.
| @@ -593,7 +612,7 @@ def bf_workspace__execute_command(self, | |||
| cmd_handler = self.fm.commands[params.command] | |||
| self._execute_request(msg_id, cmd_handler, params.arguments) | |||
|
|
|||
| def get_configuration(self, params, callback=None): | |||
| def get_configuration(self, params, callback): | |||
There was a problem hiding this comment.
Why did you remove the default value of callback here?
There was a problem hiding this comment.
get_configuration is supposed to return a value. That value is passed as an argument to the callback function when result is returned from the client. In other words, there is no reason to call this function without doing something with returned value.
| @@ -1270,8 +1282,8 @@ def __init__(self, id: str, method: str): | |||
|
|
|||
|
|
|||
| class UnregistrationParams: | |||
| def __init__(self, unregistrations: List[Unregistration]): | |||
| self.unregistration = unregistrations | |||
| def __init__(self, unregisterations: List[Unregistration]): | |||
There was a problem hiding this comment.
Renamed unregistrations to unregisterations. Was this accidental?
There was a problem hiding this comment.
It was really hard to find why it wasn't working and the reason was that specification says "unregisterations" (!?) :)
There was a problem hiding this comment.
That must've been frustrating.
| @@ -687,6 +687,18 @@ def __init__(self, uri: str, range: 'Range'): | |||
| self.range = range | |||
|
|
|||
|
|
|||
| class LocationLink: | |||
There was a problem hiding this comment.
Should this and other changes made in order to support the current LSP specification be mentioned in the changelog?
|
I have created another issue which will be closed with this PR as well and updated the CHANGELOG. |
Description (e.g. "Related to ...", etc.)
Closes #58.
Closes #68.
This PR also closes #68 because I had to fix some field names from protocol specification when working on (#58).
Code review checklist (for code reviewer to complete)