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 TypeVars rather than specific unions #8215

Closed
wants to merge 2 commits into from

Conversation

max-sixty
Copy link
Collaborator

Also from the chars of #8208 -- this uses the TypeVars we define, which hopefully sets a standard and removes ambiguity for how to type functions. It also allows subclasses.

Where possible, it uses T_Xarray, otherwise T_DataArrayOrSet where it's not possible for mypy to narrow the concrete type down.

Also from the chars of pydata#8208 -- this uses the TypeVars we define, which hopefully sets a standard and removes ambiguity for how to type functions. It also allows subclasses.

Where possible, it uses `T_Xarray`, otherwise `T_DataArrayOrSet` where it's not possible for `mypy` to narrow the concrete type down.
@max-sixty max-sixty changed the title Use typevars rather than specific unions Use TypeVars rather than specific unions Sep 19, 2023
@max-sixty
Copy link
Collaborator Author

The failure is coming from the docs — getting lots of timeouts; I think unrelated to the code changes

@max-sixty max-sixty added the plan to merge Final call for comments label Sep 20, 2023
Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

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

I'm not really a huge fan of using TypeVars if you don't have a return type that uses the same TypeVar.
For this you could also simply define a TypeAlias.

But you are right, at least it unifies things.

@max-sixty
Copy link
Collaborator Author

Yeah, fair.

Your call on whether it's an improvement.

I think if we just have DataArray | Dataset around the place, that's totally fine. The thing I was less keen on is a bunch of different ones that then mean we're digging through mypy errors to understand the cause of something. But actually that hasn't been an issue so far. So no objections to closing...

@max-sixty max-sixty removed the plan to merge Final call for comments label Sep 20, 2023
@max-sixty
Copy link
Collaborator Author

Closing — thanks for the review & useful feedback @headtr1ck , greatly appreciated!

@max-sixty max-sixty closed this Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants