openlawlibrary / pygls

A pythonic generic language server
https://pygls.readthedocs.io/en/latest/
Apache License 2.0
588 stars 105 forks source link

WorkspaceFoldersServerCapabilities.change_notifications is always sent as string #155

Closed muffinmad closed 3 years ago

muffinmad commented 3 years ago
Python 3.7.9 (default, Sep  9 2020, 00:09:07) 
[Clang 11.0.3 (clang-1103.0.32.62)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from pygls.lsp.types import WorkspaceFoldersServerCapabilities
>>> WorkspaceFoldersServerCapabilities(supported=True, change_notifications=True)
WorkspaceFoldersServerCapabilities(supported=True, change_notifications='True')
danixeee commented 3 years ago

@muffinmad There's an issue with unions in pydantic.

It happened here as well: https://github.com/openlawlibrary/pygls/blob/master/tests/lsp/test_rename.py#L161.

But in this case, changing Union[str, bool] to Union[bool, str] will fix it.

With fix:

>>> from pygls.lsp.types import WorkspaceFoldersServerCapabilities
>>> WorkspaceFoldersServerCapabilities(supported=True, change_notifications=True)
WorkspaceFoldersServerCapabilities(supported=True, change_notifications=True)                    <- Works fine
>>> WorkspaceFoldersServerCapabilities(supported=True, change_notifications='test')
WorkspaceFoldersServerCapabilities(supported=True, change_notifications='test')                   <- Works fine
>>> WorkspaceFoldersServerCapabilities(supported=True, change_notifications='True')
WorkspaceFoldersServerCapabilities(supported=True, change_notifications=True)                    <- Serialization bug
>>> 

Since changeNotifications represents ID, this won't be an issue anymore...

/**
  * Whether the server wants to receive workspace folder
  * change notifications.
  *
  * If a string is provided, the string is treated as an ID
  * under which the notification is registered on the client
  * side. The ID can be used to unregister for these events
  * using the `client/unregisterCapability` request.
*/
changeNotifications?: string | boolean;

I will check Unions in other places as well.