issue_comments: 840534009

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/5285#issuecomment-840534009 https://api.github.com/repos/pydata/xarray/issues/5285 840534009 MDEyOklzc3VlQ29tbWVudDg0MDUzNDAwOQ== 15079414 2021-05-13T12:51:16Z 2021-05-13T12:51:16Z CONTRIBUTOR

Thanks @mathause for the review.

Looks good overall. There are two things I am unsure about:

  1. You change the signature of ds.plot.scatter etc.. I think that's a worthwhile change but it needs a deprecation cycle - unless I am missing something? I'd suggest not doing that here but in a new PR. (I am also not entirely sure how to best do it...).

Yes, I suppose it would break it for anyone who was using ax positionally, but I am pretty sure no xarray docs examples do this, and I feel that the precedent from pandas, etc. is that ax is a kwarg.

  1. I see the matplotlib: part of some of the references for the first time (e.g. py:meth:`matplotlib:matplotlib.figure.Figure.add_subplot\`) - this is used very inconsistently (already before). I am not sure what's correct.

It works both ways (I've been building locally to check things), but I can certainly add in the matplotlib:s if you would like. I suppose that makes it more clear that it is an external name, not part of xarray.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   883153591