issue_comments

Thanks @crusaderky !

Thanks @max-sixty and @crusaderky !

(AppVeyor fail looks unrelated)

Hello @crusaderky! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2019-06-25 18:14:47 UTC

I was considering adding this into pep8speaks, but I think this fundamentally incompatible with pep8speak's design. It runs a handful of pre-specified linters, without actually installing/running user code. But something like mypy needs to actually have xarray and its dependencies installed to work.

Yes, good point.

I haven't spent much time with the GitHub Checks, but potentially could be a good fit - one issue with the simpler "Update the comment" model is that it doesn't encode to GH whether it's safe to merge, so projects need to duplicate the test in CI

By the way, I think I can write a script based on pep8speaks that writes/updates GitHub comments from within from Travis or Azure Pipelines. We could use that for surfacing mypy results in pull requests. I will probably give that a try over the next few days....

That would be v cool. I'm kinda surprised something like pep8speaks doesn't do this already tbh...

I was considering adding this into pep8speaks, but I think this fundamentally incompatible with pep8speak's design. It runs a handful of pre-specified linters, without actually installing/running user code. But something like mypy needs to actually have xarray and its dependencies installed to work.

By the way, I think I can write a script based on pep8speaks that writes/updates GitHub comments from within from Travis or Azure Pipelines. We could use that for surfacing mypy results in pull requests. I will probably give that a try over the next few days....

That would be v cool. I'm kinda surprised something like pep8speaks doesn't do this already tbh...

By the way, I think I can write a script based on pep8speaks that writes/updates GitHub comments from within from Travis or Azure Pipelines. We could use that for surfacing mypy results in pull requests. I will probably give that a try over the next few days....

Ah it works! Didn't know permissions transferred to forks.

@crusaderky hope that's OK, not wanting to step on your toes here...

@max-sixty do you want to push those changes to @crusaderky's branch? As a maintainer for xarray, I think you should have push permissions.

Great, I've resolved those comments @shoyer , thanks: https://github.com/max-sixty/xarray/tree/annotations

@crusaderky we should be ready to go - maybe take a glance over to ensure you're happy.

I added a whatsnew with both of us - hope that's OK

Cool, I think it should be a straight fast-forward merge from my branch to yours, and then an easy merge to master

Sorry I didn't have time last week - I'll try merging into the pr ASAP. I'm in favour of merging to mainline too

Thoughts on merging this now and continuing off master?

+1 else the merge conflicts will just pile up.

Just fixed some more merge conflicts over at https://github.com/max-sixty/xarray/tree/annotations.

Thoughts on merging this now and continuing off master?

@crusaderky I updated my branch slightly: - Couple of merge conflicts - Couple of issues around dict / OrderedDict. I tried to stay as close as possible while making the tests pass (I don't think I undid any of your most recent changes - if I did it was a mistake - feel free to modify) - I wonder whether we should merge this and then continue; I worry we'll get a constant tax of merge conflicts given how many places this touches

@max-sixty you can keep adding type info to all functions in dataset.py. I don't think I'll have time to touch it next week anyway...

Great, thanks @crusaderky ! Let me know if I can do anything to help!

@max-sixty I merged your branch in and did some tweaks to the OrderedDict's. Now moving to add more typing info to dataset.py...

@max-sixty thanks for the work! I'll go through it ASAP

@crusaderky I finished this off as best I could. Tests pass though needs a review. Here's the code: https://github.com/max-sixty/xarray/tree/annotations

(I tried pasting a patch, but I my git-fu isn't enough to exclude all the master code given there's a merge commit in there)

@crusaderky let's discuss that over in https://github.com/pydata/xarray/issues/2292. For now, I would try to defer making a decision on non-string dimension/variable names, even though that means we will have less informative type annotations.

DataArray finished; unit tests (hopefully) successful. Will now move to Dataset. Would appreciate an official, final decision from @shoyer et. al. on Hashable vs. str before I continue - also in light of all the hacks Hashable forced me to go through.

Thanks @crusaderky , this looks great!

Re:

Switching all variable names from str to Hashable. Without proper unit tests however (out of scope) non-string hashables are expected not work most of the times. My preference would still be to stay limited on str...

Curious on others' thoughts. I would vote to allow Hashable for data variable names in Datasets - similar to other Mappings - but less so for dimension names in arrays. (Not opposed to it per se, open to instances where that would be helpful)

One of the many reasons why I'm strongly against generic Hashables instead of strings is that I expect user code all over the world to have string-specific functions and methods applied to variable names and dimensions. e.g. [d for d in arr.dims if d.startswith('foo')] The above will produce an error in mypy, and the user will be forced to either disable the test or verbosely convince mypy with a typing.cast that actually in his project he never uses non-string dims.