Skip to content

Conversation

@vkverma9534
Copy link

@vkverma9534 vkverma9534 commented Dec 21, 2025

This PR fixes an invalid runtime type check that used the PEP 604 union syntax (Dataset | DataArray) inside isinstance(), which is not supported by Python and raises a TypeError. The check is updated to use a tuple of classes instead, ensuring correct detection of xarray Dataset and DataArray inputs while preserving the intended behavior.

I am happy to add parametrized Test cases if directed. Thank You!

This PR fixes an invalid runtime type check that used the PEP 604 union syntax (Dataset | DataArray) inside isinstance(), which is not supported by Python and raises a TypeError. The check is updated to use a tuple of classes instead, ensuring correct detection of xarray Dataset and DataArray inputs while preserving the intended behavior.
@max-sixty
Copy link
Collaborator

yes, would need a test (I'm not sure it's correct?)

@vkverma9534
Copy link
Author

Screenshot 2025-12-22 080307

Thanks for the review! I’ve added a regression test that exercises this code path and verifies the behavior at runtime (see screenshot above). I also noticed the same isinstance(..., Dataset | DataArray) pattern in a few nearby lines; would you like me to apply the same fix there as well?

@vkverma9534
Copy link
Author

@max-sixty Although PEP 604 allows using union types (X | Y) in isinstance() starting with Python 3.10, relying on this behavior is not safe for library runtime code. It is not backward-compatible with older Python versions, has more edge cases in runtime type checking, and is less predictable across tooling and environments. Using the tuple form isinstance(x, (A, B)) remains the safest and most portable approach.

@max-sixty
Copy link
Collaborator

appreciate the interest, but we are fine with this syntax

@max-sixty max-sixty closed this Dec 22, 2025
@vkverma9534
Copy link
Author

vkverma9534 commented Dec 22, 2025

@max-sixty Thanks for taking the time to review and explain the project’s direction. I appreciate the feedback and discussion — happy to contribute again in the future if there’s a better fit.

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.

2 participants