issue_comments

Table actions

11 rows where issue = 962647570 sorted by updated_at descending

✎ View and edit SQL

This data as json, CSV (advanced)

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

author_association 2

issue 1

  • Add typing to the OPTIONS dict · 11 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
902430683 https://github.com/pydata/xarray/pull/5678#issuecomment-902430683 https://api.github.com/repos/pydata/xarray/issues/5678 IC_kwDOAMm_X841yf_b Illviljan 14371165 2021-08-20T04:57:29Z 2021-08-20T04:57:29Z MEMBER

Yes! Literal and TypeGuard seems like great ways of narrowing down types without using slow isinstance checks everywhere.

Some nice reading: https://adamj.eu/tech/2021/05/17/python-type-hints-how-to-narrow-types-with-isinstance-assert-literal/ https://adamj.eu/tech/2021/06/09/python-type-hints-how-to-narrow-types-with-typeguard/

 
{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add typing to the OPTIONS dict 962647570
902287190 https://github.com/pydata/xarray/pull/5678#issuecomment-902287190 https://api.github.com/repos/pydata/xarray/issues/5678 IC_kwDOAMm_X841x89W max-sixty 5635139 2021-08-19T22:15:34Z 2021-08-19T22:17:14Z MEMBER

Thanks @Illviljan !

Please feel free to add a whatsnew in another PR, maybe as a batch for some of your PRs :)

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add typing to the OPTIONS dict 962647570
902287027 https://github.com/pydata/xarray/pull/5678#issuecomment-902287027 https://api.github.com/repos/pydata/xarray/issues/5678 IC_kwDOAMm_X841x86z max-sixty 5635139 2021-08-19T22:15:15Z 2021-08-19T22:15:15Z MEMBER

This looks really good!

I didn't know about Literal, TIL. We could use that in more places too, I guess.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add typing to the OPTIONS dict 962647570
902014311 https://github.com/pydata/xarray/pull/5678#issuecomment-902014311 https://api.github.com/repos/pydata/xarray/issues/5678 IC_kwDOAMm_X841w6Vn Illviljan 14371165 2021-08-19T15:32:58Z 2021-08-19T15:32:58Z MEMBER

I think this is ready for review now.

The only thing missing now is a typing for positive-only ints. I haven't figured out how to do that though.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add typing to the OPTIONS dict 962647570
894201955 https://github.com/pydata/xarray/pull/5678#issuecomment-894201955 https://api.github.com/repos/pydata/xarray/issues/5678 IC_kwDOAMm_X841THBj github-actions[bot] 41898282 2021-08-06T11:41:04Z 2021-08-14T22:24:01Z CONTRIBUTOR

Unit Test Results

6 files           6 suites   57m 36s :stopwatch: 16 217 tests 14 481 :heavy_check_mark: 1 735 :zzz: 1 :x: 90 498 runs  82 316 :heavy_check_mark: 8 176 :zzz: 6 :x:

For more details on these failures, see this check.

Results for commit c11aac2f.

:recycle: This comment has been updated with latest results.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add typing to the OPTIONS dict 962647570
894545722 https://github.com/pydata/xarray/pull/5678#issuecomment-894545722 https://api.github.com/repos/pydata/xarray/issues/5678 IC_kwDOAMm_X841Ua86 max-sixty 5635139 2021-08-06T22:17:16Z 2021-08-06T22:17:16Z MEMBER

Yeah, we had similar issues in https://github.com/pydata/xarray/pull/5624

But I think it's fine if it's messy for now given there's a clear path to clean it up when we move to 3.8.

 
{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add typing to the OPTIONS dict 962647570
894540803 https://github.com/pydata/xarray/pull/5678#issuecomment-894540803 https://api.github.com/repos/pydata/xarray/issues/5678 IC_kwDOAMm_X841UZwD Illviljan 14371165 2021-08-06T22:03:32Z 2021-08-06T22:03:32Z MEMBER

I found TypedDict in typing_extensions as well. So now I think the mypy should behave the same for all python versions. It got quite messy because typing_extensions isnt a dependency of xarray, see #5495.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add typing to the OPTIONS dict 962647570
894483982 https://github.com/pydata/xarray/pull/5678#issuecomment-894483982 https://api.github.com/repos/pydata/xarray/issues/5678 IC_kwDOAMm_X841UL4O Illviljan 14371165 2021-08-06T19:50:12Z 2021-08-06T19:53:15Z MEMBER

@max-sixty is there a way turn off the typing, but still having T_Options defined? I think having no typing is better than adding typing in the 3.7 case. Maybe I should just wrap it inside the if check as well?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add typing to the OPTIONS dict 962647570
894446124 https://github.com/pydata/xarray/pull/5678#issuecomment-894446124 https://api.github.com/repos/pydata/xarray/issues/5678 IC_kwDOAMm_X841UCos max-sixty 5635139 2021-08-06T18:35:59Z 2021-08-06T18:35:59Z MEMBER

As long as it passes mypy on 3.7, I think that's fine. If we're not sure, it's worth confirming that it does pass, either locally or adding a pre-commit task...

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add typing to the OPTIONS dict 962647570
894434509 https://github.com/pydata/xarray/pull/5678#issuecomment-894434509 https://api.github.com/repos/pydata/xarray/issues/5678 IC_kwDOAMm_X841T_zN Illviljan 14371165 2021-08-06T18:15:37Z 2021-08-06T18:15:37Z MEMBER

I'm a little uncertain about the 3.7 solution and what happens once we start using these typings more (for example is sum(int, Any) ok?).

But we can drop 3.7 in december I think so maybe these typing inconsistencies is fine?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add typing to the OPTIONS dict 962647570
894373712 https://github.com/pydata/xarray/pull/5678#issuecomment-894373712 https://api.github.com/repos/pydata/xarray/issues/5678 IC_kwDOAMm_X841Tw9Q max-sixty 5635139 2021-08-06T16:26:37Z 2021-08-06T16:26:37Z MEMBER

This looks cool! You're ahead of me on the new python features @Illviljan !

Any thoughts before we merge?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Add typing to the OPTIONS dict 962647570

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]);