fix: True LSP Diagnostic param, with version

[tombh]

Fixes #211

Includes deprecation warning for old method signature

I don't know if I'm being too clever here?

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. 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.

Code review checklist (for code reviewer to complete)

• Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
• Title summarizes what is changing
• Commit messages are meaningful (see this for details)
• Tests have been included and/or updated, as appropriate
• Docstrings have been included and/or updated, as appropriate
• Standalone docs have been updated accordingly
• CONTRIBUTORS.md was updated, as appropriate
• Changelog has been updated, as needed (see CHANGELOG.md)

[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.
2. If this type had been used to begin with, then the bug in Add an optional version parameter to LanguageServerProtocol publish_diagnostics #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

server.publish_diagnostics("file:///...", [...], version=1)

over

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

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 🙂

[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 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 either. Future-proofing is probaly not an issue. Microsoft is unlike to break what works now. They may be adding stuff but it will be backward compatible and could be accommodated, if and when necessary.

[tombh]

Ok, I've pushed a change, that incorporates both your ideas. Thank you. I still don't agree about the utility of having simpler server.LanguageServer methods, but it's a minor issue, and we've got a good compromise now, with the native LSP types being used in protocol.LanguageServerProtocol.

An nice idea @alcarney, I've made a new issue to explore finding parity between server and protocol: #306

[alcarney]

Apologies for the delay, hopefully you've all had / are enjoying the holiday break!

I made a comment, but up to you on whether you want to act on it or not.

[alcarney]

Just a thought.

It might make sense to have this method construct and pass a PublishDiagnosticsParams object, so that the user doesn't see a deprecation warning on what appears to be an internal function.

But perhaps that can wait until we decide on #306

[tombh]

This is a really good point. I've just force-pushed this. Is it what you meant?

(Apologies for the delay too! I'm in South America at the moment so it was hooooot holiday season 🥵 hahah)

[alcarney]

Looks good :)

[tombh]

Thanks :)

That docs PR triggered a conflict, so need a re-review 😬

[tombh]

Released in https://pypi.org/project/pygls/1.0.1