openlawlibrary / pygls

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

Fix `MethodTypeNotRegisteredError` #339

Closed alcarney closed 1 year ago

alcarney commented 1 year ago

Description (e.g. "Related to ...", etc.)

pygls will no longer incorrectly raise a MethodTypeNotRegisteredError when registering a TEXT_DOCUMENT_DID_SAVE feature with options.

This commit also introduces test_register_feature which supercedes the tests in tests/test_capabilities.py. It ensures that the feature manager can be used to register a method with options and that those options are used to correctly fill out the corresponding parts of the ServerCapabilities object

Fixes the issue raised in #338

Code review checklist (for code reviewer to complete)

tombh commented 1 year ago

I concur with @mturoci, first class OSS experience! ❤️ Quick and detailed responses and fix, aaaand even more tests added than are needed to verify the fix. Thank you 🥰🙇

tombh commented 1 year ago

@alcarney do you think we're seeing a flakey test with Pyodide?

https://github.com/openlawlibrary/pygls/actions/runs/5216874471/jobs/9416063934#step:5:276

alcarney commented 1 year ago
/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1050: in _gcd_import
    ???
<frozen importlib._bootstrap>:1027: in _find_and_load
    ???
<frozen importlib._bootstrap>:1006: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:688: in _load_unlocked
    ???
/lib/python3.10/site-packages/_pytest/assertion/rewrite.py:168: in exec_module
    exec(co, module.__dict__)
/lib/python3.10/site-packages/pygls/tests/conftest.py:26: in <module>
    from pygls.feature_manager import FeatureManager
/lib/python3.10/site-packages/pygls/feature_manager.py:28: in <module>
    from pygls.lsp import get_method_options_type, is_instance
/lib/python3.10/site-packages/pygls/lsp/__init__.py:38: in <module>
    from typeguard import check_type
/lib/python3.10/site-packages/typeguard/__init__.py:4: in <module>
    from ._checkers import TypeCheckerCallable as TypeCheckerCallable
/lib/python3.10/site-packages/typeguard/_checkers.py:68: in <module>
    from typing_extensions import Any as SubclassableAny
E   ImportError: cannot import name 'Any' from 'typing_extensions' (/lib/python3.10/site-packages/typing_extensions.py)

That is strange… an import error like that is pretty fundamental.

It looks like Any was added to typing_extensions in v4.4.0 so I can only guess, for whatever reason, this particular test run decided to pull in a version older than that.

It’s a shame the test runner doesn’t show the install logs from within the pyodide runtime itself…

tombh commented 1 year ago

Oh, that's interesting. So maybe can that can be fixed with a lower bound pin? I re-ran that CI run and it everything was green. So it's not an urgent issue for now, just something to watch out for.