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 type hints for generators and accessors #128

Merged
merged 4 commits into from
Dec 2, 2022
Merged

Conversation

maxrjones
Copy link
Member

Description of proposed changes

Making it easier to catch errors, for example for dataset/dataarray differences.

Fixes #

@maxrjones maxrjones requested a review from jhamman November 18, 2022 20:40
@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

Codecov Report

Merging #128 (022ebf7) into main (e976d76) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
+ Coverage   97.97%   97.98%   +0.01%     
==========================================
  Files           5        5              
  Lines         198      199       +1     
  Branches       35       34       -1     
==========================================
+ Hits          194      195       +1     
  Misses          4        4              
Impacted Files Coverage Δ
xbatcher/accessors.py 100.00% <100.00%> (ø)
xbatcher/generators.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.


def _iterate_batch_dims(self, ds):
return _iterate_through_dataset(ds, self.batch_dims)
def _iterate_batch_dims(self) -> Any:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def _iterate_batch_dims(self) -> Any:
def _iterate_batch_dims(self) -> Generator[Dict[str, int], None, None]:

I didn't test this but I think it is basically right. If it works, you can propagate this to the _iterate_through_dataset function as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, you can use the Iterator type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Lots of errors using def _iterate_batch_dims(self) -> Iterator[Dict[Hashable, slice]]:.

xbatcher/generators.py:182: error: Keywords must be strings
xbatcher/generators.py:182: error: Argument 1 to "isel" of "Dataset" has incompatible type "**Dict[Hashable, slice]"; expected "Optional[Mapping[Any, Any]]"
xbatcher/generators.py:182: error: Argument 1 to "isel" of "Dataset" has incompatible type "**Dict[Hashable, slice]"; expected "bool"
xbatcher/generators.py:182: error: Argument 1 to "isel" of "Dataset" has incompatible type "**Dict[Hashable, slice]"; expected "Literal['raise', 'warn', 'ignore']"
xbatcher/generators.py:182: error: Argument 1 to "isel" of "DataArray" has incompatible type "**Dict[Hashable, slice]"; expected "Optional[Mapping[Any, Any]]"
xbatcher/generators.py:182: error: Argument 1 to "isel" of "DataArray" has incompatible type "**Dict[Hashable, slice]"; expected "bool"
xbatcher/generators.py:182: error: Argument 1 to "isel" of "DataArray" has incompatible type "**Dict[Hashable, slice]"; expected "Literal['raise', 'warn', 'ignore']"
xbatcher/generators.py:189: error: Argument 1 to "list" has incompatible type "Iterator[Dict[Hashable, slice]]"; expected "Iterable[List[Dict[Hashable, slice]]]"
Found 8 errors in 1 file (checked 20 source files)

I have a separate refactor almost finished that would fix #131 and make this function obsolete. Do view it as necessary to type all functions before merging in the existing type hints? My preference would be to not spend more time on this.

@jhamman
Copy link
Contributor

jhamman commented Nov 28, 2022

@maxrjones - this is great. Is mypy active in one of the CI environments? I didn't see it but I could have missed it.

@maxrjones
Copy link
Member Author

@maxrjones - this is great. Is mypy active in one of the CI environments? I didn't see it but I could have missed it.

Thanks for the reviews @jhamman! Mypy is activate via the pre-commit CI (e.g., https://results.pre-commit.ci/run/github/155272397/1668812992.ah7CK-WJSvCUCBL9ANdLhA).

@jhamman jhamman merged commit b55972d into main Dec 2, 2022
@jhamman jhamman deleted the generators-typing branch December 2, 2022 18:33
@maxrjones maxrjones added the maintenance Maintenance tasks label Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants