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

Refactor to add type annotations for s_expr.Expression #7166

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

aljazerzen
Copy link
Contributor

@aljazerzen aljazerzen commented Apr 8, 2024

In cases where we use field getter on schema objects, we have a custom mypy extension that figures out the type of the field. This does not work with Pylance unfortunately, so I went around and added type annotations.

- expr = pointer.get_expr(schema)
+ expr: Optional[s_expr.Expression] = pointer.get_expr(schema)

Copy link
Member

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

I guess?

I'm not against adding any of the specific annotations, but I don't think we want to do this universally

@elprans
Copy link
Member

elprans commented Apr 11, 2024

What is the actual issue here? I disabled pylance complaints about this specifically in pyproject.toml. You should be running ms-python.mypy-type-checker in VSCode also to get proper typechecking for these (though not autocomplete sadly).

@aljazerzen
Copy link
Contributor Author

I've used LSP "rename property" action, which didn't find all usages of the property.
I know that mypy works and I do have it in vscode, but there is no "go to definition", "rename property" or "find usages".

@aljazerzen aljazerzen merged commit 5322ffb into master Apr 12, 2024
23 checks passed
@aljazerzen aljazerzen deleted the refactor-expr-annotations branch April 12, 2024 12:00
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.

None yet

3 participants