Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use generated types for python 38 #2500

Merged
merged 24 commits into from
Aug 20, 2024

Conversation

predragnikolic
Copy link
Member

CompletionItemDefaults = __CompletionList_itemDefaults_Type_1
CompletionEditRange = __CompletionList_itemDefaults_editRange_Type_1

@predragnikolic predragnikolic marked this pull request as draft June 27, 2024 20:56
@predragnikolic predragnikolic marked this pull request as ready for review June 28, 2024 11:33
plugin/core/sessions.py Outdated Show resolved Hide resolved
plugin/core/edit.py Outdated Show resolved Hide resolved
plugin/core/protocol.py Outdated Show resolved Hide resolved
plugin/core/protocol.py Show resolved Hide resolved
plugin/core/signature_help.py Outdated Show resolved Hide resolved
plugin/core/protocol.py Outdated Show resolved Hide resolved
plugin/core/edit.py Outdated Show resolved Hide resolved


def is_text_edit(value: Any) -> TypeGuard[TextEdit]:
return isinstance(value, dict) and bool(value.get('range')) and bool(value.get('newText'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's not correct. Use in to check for existance of property and not bool

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But also this does not exclude AnnotatedTextEdit if that was your goal. You'd need to explicitly check for lack of annotationId

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll push a fix next week when I get back to my laptop

Copy link
Member

@rchl rchl Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, without reading the spec because I'm lazy, do we really have to filter those out?

It would be rather silly for the spec to introduce AnnotatedTextEdit and break all the existing implementations. The implementations that are not aware of the new text edit type can not automagically filter it out. I would expect there to be a client capability for this and the server only to send this new text edit type if client says that it's supported.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also true, I did the filtering to make pyright happy. There is probably a better way to handle this, than the way I did.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's not correct. Use in to check for existance of property and not bool

Thanks, this is the reason for this bug reported here #2500 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to extend the type:

- WorkspaceChanges = dict[str, tuple[list[TextEdit], Optional[int]]]
+ WorkspaceChanges = dict[str, tuple[list[TextEdit | AnnotatedTextEdit | SnippetTextEdit], Optional[int]]]

I quit from changing the code because:

  • there is this error which I cant seem to figure out how to solve, and not cause type issues on other places ->
// Packages/LSP/plugin/core/edit.py:
32:17  error   Argument of type "tuple[List[TextEdit], None]" cannot be assigned to parameter "value" of type "tuple[list[TextEdit | AnnotatedTextEdit | SnippetTextEdit], int | None]" in function "__setitem__" ​Pyright
                    "tuple[List[TextEdit], None]" is incompatible with "tuple[list[TextEdit | AnnotatedTextEdit | SnippetTextEdit], int | None]"
                      Tuple entry 1 is incorrect type
                        "List[TextEdit]" is incompatible with "list[TextEdit | AnnotatedTextEdit | SnippetTextEdit]"
                          Type parameter "_T@list" is invariant, but "TextEdit" is not the same as "TextEdit | AnnotatedTextEdit | SnippetTextEdit"
                          Consider switching from "list" to "Sequence" which is covariant

AnnotatedTextEdit don't require code changes, however SnippetTextEdit do require code changes because they doesn't have newText, instead they have a snippet property. I rather not extend the PR to support SnippetTextEdit, at least not in this PR. I would leave the filtering of Text edits [edit for edit in edits if is_text_edit(edit)]. I just wanted to update the types with this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AnnotatedTextEdit is defined as interface AnnotatedTextEdit extends TextEdit so it does include newText, same as TextEdit.

I can look into the error later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a change that should address it in a different way

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I like the way it was addressed

predragnikolic and others added 2 commits July 12, 2024 17:43
{'range': {'start': {'line': 1, 'character': 0}, 'end': {'line': 2, 'character': 0}}, 'newText': ''}
@predragnikolic predragnikolic merged commit fe795a1 into main Aug 20, 2024
8 checks passed
@predragnikolic predragnikolic deleted the use-generated-types-for-python-38 branch August 20, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants