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

Is it possible to drop typeguard? #409

Closed
karthiknadig opened this issue Nov 7, 2023 · 4 comments · Fixed by #411
Closed

Is it possible to drop typeguard? #409

karthiknadig opened this issue Nov 7, 2023 · 4 comments · Fixed by #411

Comments

@karthiknadig
Copy link
Contributor

This dependency often causes issues:
microsoft/vscode-black-formatter#265
microsoft/vscode-pylint#446
microsoft/vscode-isort#344

All caused by outdated version of typing extension in users environment.

lsprotocol includes the Type for the Options supported for each request. Would it be possible to use that to validate options without using typeguard?

@tombh
Copy link
Collaborator

tombh commented Nov 7, 2023

I think this issue is related? #406 "Consider removing upper bounds on dependencies"

@alcarney
Copy link
Collaborator

alcarney commented Nov 7, 2023

I'm open to the idea.

As far as I know, the only place we use it is when we check to make sure the user passes the right type to the @server.feature decorator

During the initial migration to lsprotocol I tried replacing our is_instance function (which uses typeguard) with Python's regular isinstance() function - but couldn't make it work, or at least I didn't try very hard. I can't remember the details now, but it was either throwing errors when it shouldn't have, or was letting some errors slip through.

Assuming we can figure out a replacement for is_instance I'm all for it.

@karthiknadig
Copy link
Contributor Author

@alcarney Are there tests I can verify the fix against?

@alcarney
Copy link
Collaborator

alcarney commented Nov 7, 2023

They're not comprehensive, but there are a few test cases like this one in test_feature_manager.py

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 a pull request may close this issue.

3 participants