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

Add comments on when to use which TypeVar #8212

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

max-sixty
Copy link
Collaborator

From the chars of #8208

T_Xarray = TypeVar("T_Xarray", "DataArray", "Dataset")

# Use `T_DataWithCoords` for:
# - functions that return either `DataArray` or `Dataset`, and
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for this usecase you should use T_DataArrayOrSet.
You can use this one, when the code that uses the input only uses methods that are available in DataWithCoords.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes OK. I had tried to consolidate in #8213.

Is the rule for what to use really "the code that uses the input only uses methods that are available in DataWithCoords" though? And then if we add an extra method, we change everything to use T_DataArrayOrSet, given they're not always compatible?

(Maybe this isn't an easy answer, and I appreciate your review)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried another pass. If it's not quite there yet, feel free to just rewrite and/or push off for the moment

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 think now it is good and correct :)

# Maybe we rename this to T_Data or something less Fortran-y?
T_Xarray = TypeVar("T_Xarray", "DataArray", "Dataset")
# For working directly with `DataWithCoords`. It will only allow using methods defined
# on `DataWithCoords`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's already good.
Personally I have found it better to use than T_DataArrayOrSet because it can better resolve the unions. But ofc it only works under the limitations above.

@max-sixty max-sixty merged commit 1d2e5c6 into pydata:main Sep 20, 2023
@mathause
Copy link
Collaborator

Thanks - I think this is very helpful!

@max-sixty max-sixty deleted the types-comments branch September 20, 2023 17:29
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.

3 participants