home / github

Menu
  • GraphQL API
  • Search all tables

issue_comments

Table actions
  • GraphQL API for issue_comments

10 rows where issue = 636611699 sorted by updated_at descending

✖
✖

✎ View and edit SQL

This data as json, CSV (advanced)

Suggested facets: reactions, created_at (date), updated_at (date)

user 5

  • nbren12 4
  • crusaderky 2
  • mathause 2
  • max-sixty 1
  • pep8speaks 1

author_association 3

  • MEMBER 5
  • CONTRIBUTOR 4
  • NONE 1

issue 1

  • Improve typehints of xr.Dataset.__getitem__ · 10 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
644639292 https://github.com/pydata/xarray/pull/4144#issuecomment-644639292 https://api.github.com/repos/pydata/xarray/issues/4144 MDEyOklzc3VlQ29tbWVudDY0NDYzOTI5Mg== crusaderky 6213168 2020-06-16T09:10:07Z 2020-06-16T09:10:07Z MEMBER

@nbren12 it seems to me that mypy is being overly aggressive when parsing the hinted code (hence why I had to put # type: ignore on it) but it is being more lax when the same code is invoked somewhere else like in my test script. Overall I suspect it may be fragile and break in future mypy versions...

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Improve typehints of xr.Dataset.__getitem__ 636611699
644231473 https://github.com/pydata/xarray/pull/4144#issuecomment-644231473 https://api.github.com/repos/pydata/xarray/issues/4144 MDEyOklzc3VlQ29tbWVudDY0NDIzMTQ3Mw== nbren12 1386642 2020-06-15T16:15:58Z 2020-06-15T16:32:24Z CONTRIBUTOR

@crusaderky Thanks for the re-work. For my own benefit, could you explain why that code worked? I remember writing something very similar, and running into mypy errors. My understanding of how mypy intreprets overload seems incomplete.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Improve typehints of xr.Dataset.__getitem__ 636611699
643655275 https://github.com/pydata/xarray/pull/4144#issuecomment-643655275 https://api.github.com/repos/pydata/xarray/issues/4144 MDEyOklzc3VlQ29tbWVudDY0MzY1NTI3NQ== crusaderky 6213168 2020-06-13T17:44:43Z 2020-06-13T17:44:43Z MEMBER

I took the liberty to rework it, please have a look Test script:

```python from typing import Hashable, Mapping import xarray ds: xarray.Dataset

class D(Hashable, Mapping): def hash(self): ... def getitem(self, item): ... def iter(self): ... def len(self): ...

reveal_type(ds["foo"]) reveal_type(ds[["foo", "bar"]]) reveal_type(ds[{}]) reveal_type(ds[D()]) mypy output: t1.py:12: note: Revealed type is 'xarray.core.dataarray.DataArray' t1.py:13: note: Revealed type is 'xarray.core.dataset.Dataset' t1.py:14: note: Revealed type is 'xarray.core.dataset.Dataset' t1.py:15: note: Revealed type is 'xarray.core.dataset.Dataset' ```

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Improve typehints of xr.Dataset.__getitem__ 636611699
642320316 https://github.com/pydata/xarray/pull/4144#issuecomment-642320316 https://api.github.com/repos/pydata/xarray/issues/4144 MDEyOklzc3VlQ29tbWVudDY0MjMyMDMxNg== pep8speaks 24736507 2020-06-10T23:33:46Z 2020-06-13T17:43:56Z NONE

Hello @nbren12! 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 2020-06-13 17:43:56 UTC
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Improve typehints of xr.Dataset.__getitem__ 636611699
643506298 https://github.com/pydata/xarray/pull/4144#issuecomment-643506298 https://api.github.com/repos/pydata/xarray/issues/4144 MDEyOklzc3VlQ29tbWVudDY0MzUwNjI5OA== nbren12 1386642 2020-06-12T22:23:56Z 2020-06-12T22:23:56Z CONTRIBUTOR

No problem! I think I am done with this one unless you think its important that I document or test this somehow. Can someone review it?

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Improve typehints of xr.Dataset.__getitem__ 636611699
643303946 https://github.com/pydata/xarray/pull/4144#issuecomment-643303946 https://api.github.com/repos/pydata/xarray/issues/4144 MDEyOklzc3VlQ29tbWVudDY0MzMwMzk0Ng== max-sixty 5635139 2020-06-12T14:34:01Z 2020-06-12T14:34:01Z MEMBER

Nice find @mathause ; I remember that discussion now.

As @nbren12 said, this doesn't go all the way given mypy's restrictions, but seems like a dominant improvement.

Test failures are unrelated.

Thanks @nbren12 !

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Improve typehints of xr.Dataset.__getitem__ 636611699
643242850 https://github.com/pydata/xarray/pull/4144#issuecomment-643242850 https://api.github.com/repos/pydata/xarray/issues/4144 MDEyOklzc3VlQ29tbWVudDY0MzI0Mjg1MA== mathause 10194086 2020-06-12T12:24:20Z 2020-06-12T12:24:20Z MEMBER

Seems this was already discussed in GH3210 (comment) and see also the TODO:

https://github.com/pydata/xarray/blob/8f688ea92ae8416ecc3e18f6e060dad16960e9ac/xarray/core/dataset.py#L1244-L1250

(although it is not entirely clear to me whether this is actually fixed or not)

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Improve typehints of xr.Dataset.__getitem__ 636611699
643079280 https://github.com/pydata/xarray/pull/4144#issuecomment-643079280 https://api.github.com/repos/pydata/xarray/issues/4144 MDEyOklzc3VlQ29tbWVudDY0MzA3OTI4MA== nbren12 1386642 2020-06-12T05:49:56Z 2020-06-12T05:49:56Z CONTRIBUTOR

Okay. Assuming the tests pass, I think this is ready for review. I tried adding a test, but mypy didn't seem to find problems even with code that I know doesn't work (e.g. 'a'+ 1). Is there some strategy for testing tricky type hints like this?

In any case, this code does work: ``` $ cat test_mypy.py (fv3net) import xarray as xr ds = xr.Dataset({"a": ()})

arr = ds['a'] union_obj = ds[['a']]

reveal_locals() $ mypy test_mypy.py (fv3net) test_mypy.py:8: note: Revealed local types are: test_mypy.py:8: note: arr: xarray.core.dataarray.DataArray test_mypy.py:8: note: ds: xarray.core.dataset.Dataset test_mypy.py:8: note: union_obj: Union[xarray.core.dataarray.DataArray, xarray.core.dataset.Dataset] ```

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Improve typehints of xr.Dataset.__getitem__ 636611699
643014353 https://github.com/pydata/xarray/pull/4144#issuecomment-643014353 https://api.github.com/repos/pydata/xarray/issues/4144 MDEyOklzc3VlQ29tbWVudDY0MzAxNDM1Mw== nbren12 1386642 2020-06-12T01:27:55Z 2020-06-12T01:33:41Z CONTRIBUTOR

@mathause On further consideration, I think it might not be possible to get this to work. This method has three behaviors: - Mapping -> Dataset - Hashable -> DataArray - else (List): -> Dataset

With my limited understanding of mypy, I think that any two of these is supported by overload, but I'm not sure it's possible to support all 3. I tried several different options, but maybe I am missing something.

Would a good middle ground be something like this? - Hashable -> DataArray - Any -> Union[DataArray, Dataset]

I think this would work since both the input/outputs of the first one are subtypes of the second one. It's not a complete solution, but it would solve the most common problem of ds['a'] returning a union type rather than a DataArray.

{
    "total_count": 2,
    "+1": 2,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Improve typehints of xr.Dataset.__getitem__ 636611699
642554509 https://github.com/pydata/xarray/pull/4144#issuecomment-642554509 https://api.github.com/repos/pydata/xarray/issues/4144 MDEyOklzc3VlQ29tbWVudDY0MjU1NDUwOQ== mathause 10194086 2020-06-11T10:20:33Z 2020-06-11T10:20:33Z MEMBER

The mypy check throws an error: xarray/core/dataset.py:1250: error: Overloaded function signature 2 will never be matched: signature 1's parameter type(s) are the same I think you can ignore the other failures. Should that be:

```python @overload def getitem(self, key: Hashable) -> DataArray: ...

@overload
def __getitem__(self, key: Iterable[Hashable]) -> "Dataset":
    ...

`` ? Only guessing, though - it wasAny` so there may be more options. @crusaderky

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Improve typehints of xr.Dataset.__getitem__ 636611699

Advanced export

JSON shape: default, array, newline-delimited, object

CSV options:

CREATE TABLE [issue_comments] (
   [html_url] TEXT,
   [issue_url] TEXT,
   [id] INTEGER PRIMARY KEY,
   [node_id] TEXT,
   [user] INTEGER REFERENCES [users]([id]),
   [created_at] TEXT,
   [updated_at] TEXT,
   [author_association] TEXT,
   [body] TEXT,
   [reactions] TEXT,
   [performed_via_github_app] TEXT,
   [issue] INTEGER REFERENCES [issues]([id])
);
CREATE INDEX [idx_issue_comments_issue]
    ON [issue_comments] ([issue]);
CREATE INDEX [idx_issue_comments_user]
    ON [issue_comments] ([user]);
Powered by Datasette · Queries took 11.969ms · About: xarray-datasette
  • Sort ascending
  • Sort descending
  • Facet by this
  • Hide this column
  • Show all columns
  • Show not-blank rows