openlawlibrary / pygls

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

Release v1.1.0: LSP 3.17 support #377

Closed tombh closed 11 months ago

tombh commented 11 months ago

Fixes #346 (LSP 3.17 support)

Is this all we need now for a release, to just bump the version number and all the automation takes care of the rest!? Only one way to find out 🤓

Code review checklist (for code reviewer to complete)

dimbleby commented 11 months ago

I tried running the unit tests at https://github.com/pappasam/jedi-language-server against main, and it didn't go well.

Principal problem I'm seeing is:

  File "/home/dch/pygls/pygls/capabilities.py", line 390, in _with_workspace_symbol
    value.resolve_provider = self._provider_options(WORKSPACE_SYMBOL_RESOLVE)
AttributeError: 'bool' object has no attribute 'resolve_provider'

presumably because True is the default value for self._provider_options(WORKSPACE_SYMBOL), but is not suitable here.

Similar looks to be the case in _with_diagnostic_provider().

I don't know if there are further problems behind these, I haven't tried fixing.

dimbleby commented 11 months ago

By the way - apart from having more unit tests of course - this is the sort of thing that mypy is great for catching.

if you annotated _provider_options() as:


      @overload
      def _provider_options(self, feature: str) -> Union[Any, bool]:
          ...

      @overload
      def _provider_options(self, feature: str, default: T) -> Union[Any, T]:
          ...

where T = TypeVar("T") is defined earlier in the file, then

$ mypy pygls/capabilities.py
pygls/capabilities.py:309: error: Incompatible types in assignment (expression has type "Any | bool", variable has type "DocumentOnTypeFormattingOptions | None")  [assignment]
pygls/capabilities.py:400: error: Item "bool" of "Any | bool" has no attribute "resolve_provider"  [union-attr]
pygls/capabilities.py:437: error: Item "bool" of "Any | bool" has no attribute "workspace_diagnostics"  [union-attr]
pygls/capabilities.py:438: error: Incompatible types in assignment (expression has type "Any | bool", variable has type "DiagnosticOptions | DiagnosticRegistrationOptions | None")  [assignment]
Found 4 errors in 1 file (checked 1 source file)

I realise it's quite the undertaking to get this project mypy clean from its current state, but perhaps you will find it worthwhile to chip away at it.

alcarney commented 11 months ago

Thanks for catching it before the release went out!

RossBencina commented 11 months ago

I'd argue that #383 should be merged prior to this PR, since it changes the interface to translating Positions.

tombh commented 11 months ago

Shall we release then, or wait a few more days for any feedback?

@dimbleby Totally agree about the types. I've actually started exposing all the typing errors (according to Pyright) in my editor by default: https://github.com/openlawlibrary/pygls/blob/main/pyproject.toml#L81 And I try to fix as much as possible as I'm working on other things. But I'm thinking of just doing some dedicated typing work as well.

dimbleby commented 11 months ago

I don't expect to have any further comments on this, don't wait for me!

re typing: an approach that I've seen used successfully in taking a project from lots-of-errors to clean is the ratchet:

tombh commented 11 months ago

@dimbleby that's a much better idea, ratchets only let things go in one direction, nice 😏

@alcarney rebased aaaand released....we'll soon see 🤞