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

A few opportunistic improvements to type annotations #1814

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mwaskom
Copy link
Contributor

@mwaskom mwaskom commented May 10, 2024

  • Adding a couple of type annotations to give better IDE information
  • Fixing a couple of minor annotation issues
  • Updating the inv type-stubs task to remove all existing .pyi files before running

@mwaskom mwaskom requested a review from freider May 10, 2024 21:34
@@ -27,7 +28,7 @@ def _get_environment_name(environment_name: Optional[str], resolver: Optional[Re
return config.get("environment")


class _Object:
class _Object(Generic[O]):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not totally sure that inheriting from Generic here makes sense, although it did silence the typing error we were otherwise getting here. I say that because the O TypeVar is used to represent specific subclasses, rather than a different type of object that this class can act on. What do you think @freider?

Copy link
Contributor

@freider freider May 14, 2024

Choose a reason for hiding this comment

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

Yeah it's a bit ugly, but I guess we need it if we want the class level annotations for _load and _preload to use the specific subclass :( Not sure if it's worth the extra typing complexity w/ Generic since _load and _preload are quite internal and we could add explicit typing in other places to make the code safe when using them, but I don't have a super strong feeling about it

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

2 participants