Repository: openlawlibrary/pygls

Issue:
Add an optional version parameter to LanguageServerProtocol publish_diagnostics
- The LSP v15 added an optional version parameter to the publishDiagnostics notification.
- PublishDiagnosticsParams includes version but,
- LanguageServerProtocol.publish_diagnostics is not  using version

Please make version an optional parameter of LanguageServerProtocol.publish_diagnostics so that Lsp Servers can implement this feature.

Discussion:
pyscripter: I think you need to provide default values for Optional parameters e.g.

```python 
    def publish_diagnostics(
        self,
        params_or_uri: Union[str, PublishDiagnosticsParams],
        diagnostics: Optional[List[Diagnostic]],
        version: Optional[int],
    ):
```
should be
```python 
    def publish_diagnostics(
        self,
        params_or_uri: Union[str, PublishDiagnosticsParams],
        diagnostics: Optional[List[Diagnostic]] = None,
        version: Optional[int] = None,
    ):
```

Otherwise `server.publish_diagnostics(Params)` will fail.


alcarney: Looks good... though I'm not sure why we need to deprecate the old signature, can't it just be extended with an optional `version` parameter?

Also don't forget your changelog entry :) 
pyscripter: > Looks good... though I'm not sure why we need to deprecate the old signature, can't it just be extended with an optional `version` parameter?

Agree
tombh: Oh, so you both don't think we should deprecate the old signature. I'm a bit confused by that because the only argument I can think of for not starting the process of a new signature is that maintaining the current signature is just less effort.

I think there are 2 good reasons for changing.
  1. There already exists an official LSP type for precisely this purpose: [PublishDiagnosticsParams](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#publishDiagnosticsParams).
  2. If this type had been used to begin with, then the bug in #211 would never have occurred. In a sense, the actual issue wasn't the lack of the `version` param, but rather not using the correct type. So using the correct type gives us a separation of concerns such that the responsibility of following the LSP spec is placed on the LSP procotol module, whose core purpose is to adhere to the official spec.
 
BTW, this PR gives us _both_ signatures. So there's no breaking change yet. Users can easily get rid of the warning log _and_ benefit from automatic future compatibility, by using the new signature.


alcarney: I suppose my resistance to moving from the old signature is more of a stylistic argument than a technical one.

While the new signature is a win for maintainability, I'd argue it's a step back for usability - with my `pygls` user hat on I know I would prefer typing 
```python
server.publish_diagnostics("file:///...", [...], version=1)
```
over 
```python
params = PublishDiagnosticsParams(uri="file:///...", diagnostics=[...], version=1)
server.publish_diagnostics(params)
```

It should be possible to update the existing signature and still make it future proof 
```python
def publish_diagnostics(self, uri: str, diagnostics: List[Diagnostic], version: Optional[int] = None, **kwargs):
    params = PublishDiagnosticsParams(
       uri=uri,
       diagnostics=diagnostics,
       version=version,
       **kwargs
    )
    self.lsp.publish_diagnostics(params) 
```
Yes, it's a little bit of extra maintenance for us to add in new fields to aid with discoverability, but at least no one is prevented from using the new fields as soon as they're available.

> Also, does anybody know why there are often 2 public methods to do the same thing like this? Therefore, we can call both `pygls.server.LanguageServer.publish_diagnostics` and `pygls.protocol.LanguageServerProtocol.publish_diagnostics`. 

Perhaps we can use this PR to decide what we want the answer to be? I'd argue the answer should be your assumption

> I assume it's because `pygls.server.LanguageServer` is the more friendly user-facing class, whilst `pygls.protocol.LanguageServerProtocol` is internal and has to be comprehensive.

Where the `LanguageServerProtocol` implements the spec as defined with methods accepting `params` objects and therefore always in line with the spec and future proof, leaving the `LanguageServer` methods free to reduce some of the boilerplate where possible.

Looking at `server.py` I can see things aren't consistent today, but we could at least decide what we want to aim for :slightly_smiling_face: 
 

pyscripter: > I know I would prefer typing

I only know the python jedi language server and they use server.py calls, I guess for that reason.

[My solution ](https://github.com/openlawlibrary/pygls/issues/211#issuecomment-1339546874) to this issue is along your lines, simple, and backward compatible.   

But there is a half way house which I think is preferable.   The signature at server.py stays the same (with optional version added), but the signature at protocol.py changes to PublishDiagnosticsParams.   So users have a choice to use with either.

tombh: Thanks :)

That docs PR triggered a conflict, so need a re-review 😬

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d49b73c..363a4c0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,11 +13,12 @@ and this project adheres to [Semantic Versioning][semver].
  - Fix progress example in json extension. ([#230])
  - Fix `AttributeErrors` in `get_configuration_async`, `get_configuration_callback`, `get_configuration_threaded` commands in json extension. ([#307])
  - Fix type annotations for `get_configuration_async` and `get_configuration` methods on `LanguageServer` and `LanguageServerProtocol` objects ([#307])
+ - Provide `version` param for publishing diagnostics ([#303]) 
 
 [#230]: https://github.com/openlawlibrary/pygls/issues/230
+[#303]: https://github.com/openlawlibrary/pygls/issues/303
 [#307]: https://github.com/openlawlibrary/pygls/issues/307
 
-
 ## [1.0.0] - 2/12/2022
 ### Changed
 BREAKING CHANGE: Replaced `pydantic` with [`lsprotocol`](https://github.com/microsoft/lsprotocol)
diff --git a/pygls/protocol.py b/pygls/protocol.py
index 4f0d012..5b2fcef 100644
--- a/pygls/protocol.py
+++ b/pygls/protocol.py
@@ -835,10 +835,71 @@ class LanguageServerProtocol(JsonRPCProtocol, metaclass=LSPMeta):
 
         self.notify(LOG_TRACE, params)
 
-    def publish_diagnostics(self, doc_uri: str, diagnostics: List[Diagnostic]) -> None:
-        """Sends diagnostic notification to the client."""
-        self.notify(TEXT_DOCUMENT_PUBLISH_DIAGNOSTICS,
-                    PublishDiagnosticsParams(uri=doc_uri, diagnostics=diagnostics))
+    def _publish_diagnostics_deprecator(
+        self,
+        params_or_uri: Union[str, PublishDiagnosticsParams],
+        diagnostics: Optional[List[Diagnostic]],
+        version: Optional[int],
+        **kwargs
+    ) -> PublishDiagnosticsParams:
+        if isinstance(params_or_uri, str):
+            message = "DEPRECATION: "
+            "`publish_diagnostics("
+            "self, doc_uri: str, diagnostics: List[Diagnostic], version: Optional[int] = None)`"
+            "will be replaced with `publish_diagnostics(self, params: PublishDiagnosticsParams)`"
+            logging.warning(message)
+
+            params = self._construct_publish_diagnostic_type(
+                params_or_uri,
+                diagnostics,
+                version,
+                **kwargs
+            )
+        else:
+            params = params_or_uri
+        return params
+
+    def _construct_publish_diagnostic_type(
+        self,
+        uri: str,
+        diagnostics: Optional[List[Diagnostic]],
+        version: Optional[int],
+        **kwargs
+    ) -> PublishDiagnosticsParams:
+        if diagnostics is None:
+            diagnostics = []
+
+        args = {
+            **{
+                "uri": uri,
+                "diagnostics": diagnostics,
+                "version": version
+            },
+            **kwargs
+        }
+
+        params = PublishDiagnosticsParams(**args)  # type:ignore
+        return params
+
+    def publish_diagnostics(
+        self,
+        params_or_uri: Union[str, PublishDiagnosticsParams],
+        diagnostics: Optional[List[Diagnostic]] = None,
+        version: Optional[int] = None,
+        **kwargs
+    ):
+        """
+        Sends diagnostic notification to the client.
+        Deprecation:
+          `uri`, `diagnostics` and `version` fields will be deprecated
+        """
+        params = self._publish_diagnostics_deprecator(
+            params_or_uri,
+            diagnostics,
+            version,
+            **kwargs
+        )
+        self.notify(TEXT_DOCUMENT_PUBLISH_DIAGNOSTICS, params)
 
     def register_capability(self, params: RegistrationParams,
                             callback: Optional[Callable[[], None]] = None) -> Future:
diff --git a/pygls/server.py b/pygls/server.py
index 9fd48ab..4fd6953 100644
--- a/pygls/server.py
+++ b/pygls/server.py
@@ -408,9 +408,23 @@ class LanguageServer(Server):
         """Gets the object to manage client's progress bar."""
         return self.lsp.progress
 
-    def publish_diagnostics(self, doc_uri: str, diagnostics: List[Diagnostic]):
-        """Sends diagnostic notification to the client."""
-        self.lsp.publish_diagnostics(doc_uri, diagnostics)
+    def publish_diagnostics(
+        self,
+        uri: str,
+        diagnostics: Optional[List[Diagnostic]] = None,
+        version: Optional[int] = None,
+        **kwargs
+    ):
+        """
+        Sends diagnostic notification to the client.
+        """
+        params = self.lsp._construct_publish_diagnostic_type(
+            uri,
+            diagnostics,
+            version,
+            **kwargs
+        )
+        self.lsp.publish_diagnostics(params, **kwargs)
 
     def register_capability(self, params: RegistrationParams,
                             callback: Optional[Callable[[], None]] = None) -> Future:


Test patch:
diff --git a/tests/lsp/test_diagnostics.py b/tests/lsp/test_diagnostics.py
new file mode 100644
index 0000000..a4d438a
--- /dev/null
+++ b/tests/lsp/test_diagnostics.py
@@ -0,0 +1,95 @@
+############################################################################
+# Copyright(c) Open Law Library. All rights reserved.                      #
+# See ThirdPartyNotices.txt in the project root for additional notices.    #
+#                                                                          #
+# Licensed under the Apache License, Version 2.0 (the "License")           #
+# you may not use this file except in compliance with the License.         #
+# You may obtain a copy of the License at                                  #
+#                                                                          #
+#     http: // www.apache.org/licenses/LICENSE-2.0                         #
+#                                                                          #
+# Unless required by applicable law or agreed to in writing, software      #
+# distributed under the License is distributed on an "AS IS" BASIS,        #
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. #
+# See the License for the specific language governing permissions and      #
+# limitations under the License.                                           #
+############################################################################
+
+from typing import List
+
+import time
+from lsprotocol.types import (
+    TEXT_DOCUMENT_PUBLISH_DIAGNOSTICS,
+    PublishDiagnosticsParams,
+    Diagnostic,
+    Range,
+    Position,
+)
+from ..conftest import ClientServer
+
+
+class ConfiguredLS(ClientServer):
+    def __init__(self):
+        super().__init__()
+        self.client.notifications: List[PublishDiagnosticsParams] = []
+
+        @self.client.feature(TEXT_DOCUMENT_PUBLISH_DIAGNOSTICS)
+        def f1(params: PublishDiagnosticsParams):
+            self.client.notifications.append(params)
+
+
+@ConfiguredLS.decorate()
+def test_diagnostics_notifications(client_server):
+    client, server = client_server
+    diagnostic = Diagnostic(
+        range=Range(
+            start=Position(line=0, character=0),
+            end=Position(line=1, character=1),
+        ),
+        message="test diagnostic",
+    )
+    server.lsp.publish_diagnostics(
+        PublishDiagnosticsParams(uri="", diagnostics=[diagnostic], version=1),
+    )
+    server.lsp.publish_diagnostics(
+        "",
+        diagnostics=[diagnostic],
+        version=1,
+    )
+
+    time.sleep(0.1)
+
+    assert len(client.notifications) == 2
+    expected = PublishDiagnosticsParams(
+        uri="",
+        diagnostics=[diagnostic],
+        version=1,
+    )
+    assert client.notifications[0] == expected
+
+
+@ConfiguredLS.decorate()
+def test_diagnostics_notifications_deprecated(client_server):
+    client, server = client_server
+    diagnostic = Diagnostic(
+        range=Range(
+            start=Position(line=0, character=0),
+            end=Position(line=1, character=1),
+        ),
+        message="test diagnostic",
+    )
+    server.publish_diagnostics(
+        "",
+        diagnostics=[diagnostic],
+        version=1,
+    )
+
+    time.sleep(0.1)
+
+    assert len(client.notifications) == 1
+    expected = PublishDiagnosticsParams(
+        uri="",
+        diagnostics=[diagnostic],
+        version=1,
+    )
+    assert client.notifications[0] == expected