issue_comments

Table actions

8 rows where issue = 592331420 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

  • Remove old auto combine · 8 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
607600197 https://github.com/pydata/xarray/pull/3926#issuecomment-607600197 https://api.github.com/repos/pydata/xarray/issues/3926 MDEyOklzc3VlQ29tbWVudDYwNzYwMDE5Nw== pep8speaks 24736507 2020-04-02T03:26:04Z 2020-06-24T14:38:55Z NONE

Hello @TomNicholas! 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-24 14:38:55 UTC
 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove old auto combine 592331420
611372701 https://github.com/pydata/xarray/pull/3926#issuecomment-611372701 https://api.github.com/repos/pydata/xarray/issues/3926 MDEyOklzc3VlQ29tbWVudDYxMTM3MjcwMQ== TomNicholas 35968931 2020-04-09T07:19:34Z 2020-04-09T07:19:34Z MEMBER

Just noticed there's a section of the API reference for Deprecated/Pending Deprecation, but auto_combine was never added to it...

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove old auto combine 592331420
608015240 https://github.com/pydata/xarray/pull/3926#issuecomment-608015240 https://api.github.com/repos/pydata/xarray/issues/3926 MDEyOklzc3VlQ29tbWVudDYwODAxNTI0MA== keewis 14808389 2020-04-02T18:02:12Z 2020-04-02T18:02:12Z MEMBER

thanks, @TomNicholas, looks good to me.

I opened #3928 for the upstream-dev failures.

 
{
    "total_count": 1,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 1,
    "rocket": 0,
    "eyes": 0
}
   Remove old auto combine 592331420
607979472 https://github.com/pydata/xarray/pull/3926#issuecomment-607979472 https://api.github.com/repos/pydata/xarray/issues/3926 MDEyOklzc3VlQ29tbWVudDYwNzk3OTQ3Mg== max-sixty 5635139 2020-04-02T17:18:04Z 2020-04-02T17:57:56Z MEMBER

I think FutureWarning and DeprecationWarning are pretty similar — mostly focused on whether the behavior will change or the API is going away. ~FutureDeprecationWarning~ PendingDeprecationWarning is generally lower visibility IIRC.

So maybe this should have been DeprecationWarning but no need to go through another cycle imo.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove old auto combine 592331420
608010931 https://github.com/pydata/xarray/pull/3926#issuecomment-608010931 https://api.github.com/repos/pydata/xarray/issues/3926 MDEyOklzc3VlQ29tbWVudDYwODAxMDkzMQ== keewis 14808389 2020-04-02T17:54:09Z 2020-04-02T17:57:28Z MEMBER

The warning messages said "will no longer accept" and "will require" in "version 0.15" - that's probably authoritative enough isn't it?

I'd think so, yes. Rereading what the warnings are intended for, FutureWarning is indeed the same as DeprecationWarning, the only difference is the intended target audience. I seem to have confused FutureWarning with PendingDeprecationWarning.

Then my only issue is with the whats-new.rst entry. I'd explicitly state that auto_combine was removed.

Edit: sorry, you also mentioned that. The way I understood "deprecating" was what you meant with "starting the deprecation cycle", so that's where the confusion came from.

 
{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove old auto combine 592331420
607989892 https://github.com/pydata/xarray/pull/3926#issuecomment-607989892 https://api.github.com/repos/pydata/xarray/issues/3926 MDEyOklzc3VlQ29tbWVudDYwNzk4OTg5Mg== TomNicholas 35968931 2020-04-02T17:36:19Z 2020-04-02T17:36:39Z MEMBER

To make the docs CI pass

Thanks @keewis .

Shouldn't we change the FutureWarning to a DeprecationWarning before removing auto_combine?

I wasn't even aware that there were two types of warnings I could have used for this. I was taking "deprecating" to mean "removed" and "starting the deprecation cycle" to mean "start warning people it will soon be removed". The warning messages said "will no longer accept" and "will require" in "version 0.15" - that's probably authoritative enough isn't it?

There was a case I ran in to where the old auto_combine was needed and none of the newer options worked.

I think that was always a possible edge case - I would very much like to see it though @dcherian , if only so that we can put a "what to do if" section in the docs.

 
{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove old auto combine 592331420
607980436 https://github.com/pydata/xarray/pull/3926#issuecomment-607980436 https://api.github.com/repos/pydata/xarray/issues/3926 MDEyOklzc3VlQ29tbWVudDYwNzk4MDQzNg== dcherian 2448579 2020-04-02T17:19:53Z 2020-04-02T17:19:53Z MEMBER

There was a case I ran in to where the old auto_combine was needed and none of the newer options worked.

Let me see if I can dig it up.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove old auto combine 592331420
607807193 https://github.com/pydata/xarray/pull/3926#issuecomment-607807193 https://api.github.com/repos/pydata/xarray/issues/3926 MDEyOklzc3VlQ29tbWVudDYwNzgwNzE5Mw== TomNicholas 35968931 2020-04-02T12:10:33Z 2020-04-02T12:10:33Z MEMBER

It would also be good to include any improvement to the relevant docs (e.g. #3830) in the same release.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove old auto combine 592331420

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