refactor(types): Mark properties with NotRequired #2062
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jwortmann
reviewed
Sep 20, 2022
rwols
approved these changes
Sep 21, 2022
predragnikolic
approved these changes
Sep 21, 2022
rwols
reviewed
Sep 24, 2022
| from typing import TypeVar | ||
| from typing import Union | ||
| from typing_extensions import NotRequired | ||
| from typing_extensions import Required | ||
| from typing_extensions import TypedDict |
There was a problem hiding this comment.
typing_extensions is a third-party package, so this won't work on python 3.8.
There was a problem hiding this comment.
In 3.8 we could keep using real imports for type checking and dummy ones for the plugin host, like we do now.
Then:
- this won't really matter
- if we set python version to 3.11 for pyright then we can import those from
typing
One thing I'm not sure about (although it doesn't matter for ST) is how both pyright and mypy don't have problems with this typing_extensions import. I guess that they either come with own copies or use system ones?
rchl
added a commit
that referenced
this pull request
Sep 27, 2022
* main: Fix issues with diagnostics panel toggling on save (#2063) Request color presentations when clicking on a color box (#2065) Import new imports like NotRequired from "typing" Ensure "Source Actions..." request includes the "source" kind (#2064) refactor(types): Mark properties with NotRequired (#2062) Add template variable `$text_document_position` in execute command (#2061) Only update content of diagnostics panel when visible (#2054) Remove Range class and rename RangeLsp to Range (#2058)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make use of auto-generated types from https://github.com/sublimelsp/lsp-python-types that utilize
NotRequiredfor properties that might not be present.https://github.com/sublimelsp/lsp-python-types generates two separate files -
lsp_types.pyandlsp_types_sublime_text_33.py. Former uses new class syntax that is documentation-annotations-friendly but is not compatible with Python 3.3. Latter doesn't use the new class syntax. We have to use the latter which means that documentation for properties won't be picked up by pyright but otherwise there are no downsides.Even though ST's 3.3 host doesn't support
NotRequired(or eventypingfor that matter), that doesn't matter because for 3.3 the existing LSP code has a dummy type aliases that do nothing anyway. The important part is that type checkers likemypyandpyrighttype-check using Python version that supports those types.Some notes about changes:
WorkspaceFolderclass conflicted with same-named LSP type. Moved our class toworkspace.py. Since it's a public export, plugins that imported it fromLSP.pluginwill be unaffected. The few that imported fromLSP.core.protocolwill be updated and I took care of that.Cannot resolve name "X" (possible cyclic definition). This is seemingly already supported onmaster(as an opt-in) but not released yet. For now mypy is disabled due to that. I'm not sure but I don't think we can ignore type errors for specific file as if mypy can't process those types then I don't think it would be able to type check anyway.Promise.pyrightintox.iniand CI and fixedalmostall errors that it reported.The remaining ones are the stupid cases likeview = None # type: sublime.Viewthat I think we shouldn't be doing as that looks and feels wrong. Haven't investigated how we could fix those yet. Potentially we can just ignore those.