8000 Fix some errors detected by `mypy` by dimbleby · Pull Request #100 · microsoft/lsprotocol · GitHub
[go: up one dir, main page]
Skip to content

Fix some errors detected by mypy #100

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 26, 2022
Merged

Fix some errors detected by mypy #100

merged 2 commits into from
Oct 26, 2022

Conversation

dimbleby
Copy link
Member
@dimbleby dimbleby commented Oct 20, 2022 •
edited
Loading

picking off quite a few typechecking errors.

Some remarks

  • I haven't committed the re-generated code

    • when I regenerate, several hooks go missing (the same happens on main)
    • perhaps you have a set of manual changes that you apply to the autogenerated code?

  • I believe that enable_recursive_aliases will soon be mypy default, so that seems a very reasonable thing to include

  • I haven't added py.typed, or tried to update your github workflow to run mypy

    • you'll want to do both of those I think: so that others can take advantage of these types, and so that they don't regress

remaining errors are as follows:

lsprotocol/types.py:5136: error: Name "kind" already defined on line 5130  [no-redef]
lsprotocol/types.py:5162: error: Name "kind" already defined on line 5153  [no-redef]
lsprotocol/types.py:5185: error: Name "kind" already defined on line 5179  [no-redef]
lsprotocol/types.py:12070: error: Dict entry 82 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12076: error: Dict entry 88 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12114: error: Dict entry 126 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12116: error: Dict entry 128 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12183: error: Dict entry 195 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12213: error: Dict entry 225 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12226: error: Dict entry 238 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12241: error: Dict entry 253 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12268: error: Dict entry 280 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12291: error: Dict entry 303 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12298: error: Dict entry 310 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12389: error: Dict entry 401 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12411: error: Dict entry 423 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/types.py:12534: error: Dict entry 546 has incompatible type "str": "object"; expected "str": "type"  [dict-item]
lsprotocol/converters.py:122: error: Name "LSPAny" is not defined  [name-defined]

where

  • the first group look to me as though they might represent a real bug, probably you should look into that
  • the second group are I think a mypy error, possibly Union not being recognized as types.UnionType python/mypy#13810 or similar
    • probably these just want ignoring?
  • the final one would be easy to fix - use lsp_types.LSPAny - but doing that for some reason causes test breakage
    • seems like there's something screwy here, again you might want to look into it
    • though perhaps the unusual treatment of "LspAny" in this piece of code suggests you already know that something's up

@karthiknadig
Copy link
Member

the final one would be easy to fix - use lsp_types.LSPAny - but doing that for some reason causes test breakage
seems like there's something screwy here, again you might want to look into it
though perhaps the unusual treatment of "LspAny" in this piece of code suggests you already know that something's up

It is a cattrs thing, for some reason depending on the use of TypeAlias the behavior changes. But I can't use TypeAlias as unless we move to python that shipped TypeAlias inbox. That seems to again cause issues with cattrs hooks.

@karthiknadig karthiknadig added the debt Technical debt or repo cleanup label Oct 20, 2022
@karthiknadig karthiknadig self-assigned this Oct 20, 2022
@karthiknadig karthiknadig self-requested a review October 20, 2022 18:04
@dimbleby
revert use of TypeAlias
f2aab39 
it's half-right, but also half-wrong

in the places where the definitions are used only for typechecking, this
is what is wanted

but in the ALL_TYPES_MAP, it's a lie: the runtime type of, say,
```python
DefinitionLink = "LocationLink"
```
really is a string, regardless of annotation
@karthiknadig
Copy link
Member

@dimbleby Can I merge this?

@dimbleby
Copy link
Member Author

Sure, go for it.

re the remaining errors in the ALL_TYPES_MAP, I'm no longer at all sure what the right fix is. regardless of whether the typechecker can be tricked into believing that strings and unions are types... they're not!

@karthiknadig karthiknadig changed the title typechecking Fix some errors detected by mypy Oct 26, 2022
@karthiknadig karthiknadig merged commit 0514416 into microsoft:main Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karthiknadig karthiknadig karthiknadig approved these changes

Assignees

@karthiknadig karthiknadig

Labels
debt Technical debt or repo cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dimbleby @karthiknadig
0