issue_comments: 778842603

This data as json

html_url issue_url id node_id user created_at updated_at author_association body reactions performed_via_github_app issue
https://github.com/pydata/xarray/pull/4904#issuecomment-778842603 https://api.github.com/repos/pydata/xarray/issues/4904 778842603 MDEyOklzc3VlQ29tbWVudDc3ODg0MjYwMw== 10194086 2021-02-14T21:12:02Z 2021-02-14T21:12:02Z MEMBER

Thanks for the PR & your work. I'll need some time to digest that all... One thing I certainly would like to fix before merging this is #4881 (and merge #4878). I'd also be interested in your motivation for the stub files. (It's just that we don't have any of these so far).

I am sure you saw this comment:

https://github.com/pydata/xarray/blob/3681b6508d765b1b477f51963365807a85b53d89/xarray/core/ops.py#L3-L5

so there would be an argument to create mixin-classes*. Another motivation would be that currently (with the inject approach) mypy does not know which methods DataArray actually has. This is kind of hidden by __getattr__ (see #4601 & #4616). Now I am not saying you have to do this here but also wanted to mention it.

* Well I just realised that your stub files are kind of mixin classes...

  • What are all the # type: ignore[misc] you are suppressing?
  • I was slightly confused by the T_ of T_Other etc because this is not a TypeVar (but an alias) so I'd prefer another name.
  • For T_Dataset = TypeVar("T_Dataset", bound=Dataset) etc - why do you use a bound? To allow subclassing? As we discourage subclassing we may have to discuss if we should allow this.
 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   807764700