Type hints for combine functions #5519
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…converter to combine_by_coords to check for all DataArray case and convert to datasets.
…rrays and dataset input and with empty list.
…yling tools to match the other docstrings.
…xarray into aijams/combine-by-coords
TomNicholas
commented
Jun 23, 2021
Comment on lines
+359
to
+361
| # Define type for arbitrarily-nested list of lists recursively | ||
| # Currently mypy cannot handle this but other linters can (https://stackoverflow.com/a/53845083/3154101) | ||
| DATASET_HYPERCUBE = Union[Dataset, Iterable["DATASET_HYPERCUBE"]] # type: ignore |
There was a problem hiding this comment.
The weird recursive definition is here
|
(It does pass |
|
Thanks @TomNicholas ! |
|
@TomNicholas given the upstream changes, should we merge main in first and then merge? |
snowman2
pushed a commit
to snowman2/xarray
that referenced
this pull request
Feb 9, 2022
* Added test for combine_by_coords changes. * Modified test case to expect a dataset instead of a DataArray. Added converter to combine_by_coords to check for all DataArray case and convert to datasets. * Added tests to check combine_by_coords for exception with mixed DataArrays and dataset input and with empty list. * Formatting changes after running black * Added underscore to helper function to label as private. * Black formatting changes for whats-new doc file. * Removed imports in docstring that were automatically added by code styling tools to match the other docstrings. * Removed duplicate new item line in whats-new. * combine methods now accept unnamed DataArrays as input. * combine nested test checks nested lists of unnamed DataArrays. * Made combine_by_coords more readable. * Cosmetic changes to code style. * Removed extra test from merge with previous PR. * Updated test to use pytest.raises instead of raises_regex. * Added breaking-change entry to whats new page. * Added deprecation warning to combine_coords * Removed index monotonicity checking temporarily. * Removed duplicate entries from whats new page. * Removed TODO message * Added test for combine_nested. * Added check to combine methods to clarify parameter requirements. * Reassigned description of changes to bug fixes category. * Minor style changes. * Added blank line for style purposes. * added type hints * force CI Co-authored-by: Augustus Ijams <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added type hints to
combine_nestedandcombine_by_coords.Builds on #4696 because that PR generalised the argument types to include DataArrays, but I couldn't see that branch in the list to base this PR off of.
The "nested list-of-lists" argument to
combine_nestedopens up a can of worms: the only way I can see to specify the type of a nested list of arbitrary depth is to define the type recursively, but mypy does not currently support recursive type definitions, though some other type checkers can, e.g. Microsoft's Pylance does. We're going to have the same problem when specifying types foropen_mfdataset. For now this problem is just ignored by the type checker, meaning that we don't actually check the type of the nested-list-of-lists.pre-commit run --all-filesUser visible changes (including notable bug fixes) are documented inwhats-new.rst