issue_comments

Table actions

5 rows where issue = 870619014 and user = 13301940 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 1

  • andersy005 · 5 ✖

issue 1

  • Code cleanup · 5 ✖

author_association 1

id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
839910670 https://github.com/pydata/xarray/pull/5234#issuecomment-839910670 https://api.github.com/repos/pydata/xarray/issues/5234 MDEyOklzc3VlQ29tbWVudDgzOTkxMDY3MA== andersy005 13301940 2021-05-12T16:15:13Z 2021-05-12T16:15:13Z MEMBER

I believe I have addressed most, if not all changes requested during the review. If there are still disputable changes that need addressing, please let me know 

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Code cleanup 870619014
839903564 https://github.com/pydata/xarray/pull/5234#issuecomment-839903564 https://api.github.com/repos/pydata/xarray/issues/5234 MDEyOklzc3VlQ29tbWVudDgzOTkwMzU2NA== andersy005 13301940 2021-05-12T16:07:29Z 2021-05-12T16:07:29Z MEMBER

Shall we also add parts of Stephan's comment to the "Contributing Guide" in the documentation?

👍🏽 for addressing this in a separate PR

 
{
    "total_count": 2,
    "+1": 2,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Code cleanup 870619014
839878862 https://github.com/pydata/xarray/pull/5234#issuecomment-839878862 https://api.github.com/repos/pydata/xarray/issues/5234 MDEyOklzc3VlQ29tbWVudDgzOTg3ODg2Mg== andersy005 13301940 2021-05-12T15:44:51Z 2021-05-12T15:44:51Z MEMBER

@max-sixty, I'm going to address a few issues pointed out during the review, then we can merge afterwards. 

{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Code cleanup 870619014
834820456 https://github.com/pydata/xarray/pull/5234#issuecomment-834820456 https://api.github.com/repos/pydata/xarray/issues/5234 MDEyOklzc3VlQ29tbWVudDgzNDgyMDQ1Ng== andersy005 13301940 2021-05-07T22:20:00Z 2021-05-07T22:20:00Z MEMBER

@shoyer, Thank you for the thorough feedback.

Even code that has been carefully checked can introduce bugs. Style clean-up PRs thus still need to be reviewed carefully.

Fewer lines or tokens of code is not always better. Good examples from this PR include reduced nesting with early return statements, comprehensions and conditional expressions. All of these would be unobjectionable to use in new code (and are often a good idea), but code that doesn't use them even when it could is fine, too.

👍🏽. I reverted back to the original style in most places. There're a few comments that I haven't addressed yet. I will look into those sometime this weekend.

Code-base wide clean-up for particular issues may be warranted occasionally, but I would suggest always discussing it with other core developers first.

I agree. I didn't expect the PR to get this large. My original intent when I created the PR was to clean the backends code, and I ended up invading other areas :(. In the future, I'll definitely make sure to start a discussion before working on changes that may end up affecting a large chunk of the code base. 

{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Code cleanup 870619014
833520119 https://github.com/pydata/xarray/pull/5234#issuecomment-833520119 https://api.github.com/repos/pydata/xarray/issues/5234 MDEyOklzc3VlQ29tbWVudDgzMzUyMDExOQ== andersy005 13301940 2021-05-06T13:24:18Z 2021-05-06T13:24:18Z MEMBER

I feel this PR is too big and diverse to accept it as a whole.

For example I like most changes from older style formatting to f-strings, but I disagree strongly the moving the return statements into functions improves readability.

If folks are in favor of the original style (inline variables that are immediately returned) throughout the codebase, I'm happy to revert back to the original style :) 

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

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