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

Clean up _destag_variable with respect to types and terminology #103

Merged
merged 2 commits into from
Sep 16, 2022

Conversation

jthielen
Copy link
Collaborator

@jthielen jthielen commented Sep 16, 2022

Change Summary

Further cleanups of #93 after merge (sorry about that!), this time to help out developers/contributors who wish to build upon the private _destag_variable function:

As discussed in #100 (comment) and #100 (comment), this adds a type check for xarray.Variable in _destag_variable as a safe guard for xwrf contributors against passing in an xarray.DataArray by accident. Also, updates comments in _destag_variable from the original version in #37 to have a bit more accurate terminology.

Related issue number

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable

@jthielen jthielen added this to the v0.0.2 milestone Sep 16, 2022
@jthielen jthielen requested a review from a team September 16, 2022 21:45
Copy link
Collaborator

@lpilz lpilz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jthielen jthielen force-pushed the type-check-destagger branch from afb0c87 to a6b26a3 Compare September 16, 2022 22:03
@jthielen
Copy link
Collaborator Author

Rebased on main after #100 just to be extra careful. With @lpilz review approving, I've enabled auto-merge once CI passes.

@jthielen jthielen enabled auto-merge (squash) September 16, 2022 22:05
@jthielen jthielen merged commit d3c451b into xarray-contrib:main Sep 16, 2022
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