issue_comments

Table actions

9 rows where author_association = "MEMBER" and issue = 606549469 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 2

issue 1

  • Remove dangerous default argument · 9 ✖

author_association 1

  • MEMBER · 9 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
619405476 https://github.com/pydata/xarray/issues/4002#issuecomment-619405476 https://api.github.com/repos/pydata/xarray/issues/4002 MDEyOklzc3VlQ29tbWVudDYxOTQwNTQ3Ng== shoyer 1217238 2020-04-25T16:30:07Z 2020-04-25T16:30:07Z MEMBER

Don't waste your time on that one

On Sat, Apr 25, 2020 at 9:19 AM Prajjwal Nijhara notifications@github.com wrote:

Agreed with that. So, should I open another issue for asserts or not?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/issues/4002#issuecomment-619403922, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJJFVVVF7RW5HZFKAF62X3ROMERVANCNFSM4MQL3K7A . 

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove dangerous default argument 606549469
619398262 https://github.com/pydata/xarray/issues/4002#issuecomment-619398262 https://api.github.com/repos/pydata/xarray/issues/4002 MDEyOklzc3VlQ29tbWVudDYxOTM5ODI2Mg== shoyer 1217238 2020-04-25T15:42:29Z 2020-04-25T15:42:29Z MEMBER

We do not use assert for user facing validation

On Sat, Apr 25, 2020 at 3:37 AM Prajjwal Nijhara notifications@github.com wrote:

@shoyer https://github.com/shoyer, I partially agree with your statement that " assert is the appropriate way to verify internal invariants". This is correct but not every time. When you see the link that you shared it also shows that most of the assert statements are used in tests and not in simple files.

Let me quote again "Since assert provides an easy way to check some condition and fail execution, it’s very common for developers to use it to check validity. But when the Python interpreter is invoked with the -O (optimize) flag, the assert statements are removed from the bytecode. So, if assert statements are used for user-facing validation in production code, the block won’t be executed at all — potentially opening up a security vulnerability. It is recommended to use assert statements only in tests."

Again it's just a suggestion IMHO

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/issues/4002#issuecomment-619358326, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJJFVWLOJULCHNMC5F4KZTROK4NZANCNFSM4MQL3K7A . 

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove dangerous default argument 606549469
619330330 https://github.com/pydata/xarray/issues/4002#issuecomment-619330330 https://api.github.com/repos/pydata/xarray/issues/4002 MDEyOklzc3VlQ29tbWVudDYxOTMzMDMzMA== shoyer 1217238 2020-04-25T06:33:20Z 2020-04-25T06:33:20Z MEMBER

assert is the appropriate way to verify internal invariants. We use it routinely at Google and you can also find many examples in Python’s standard library: https://github.com/python/cpython/search?l=Python&q=assert

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove dangerous default argument 606549469
619322526 https://github.com/pydata/xarray/issues/4002#issuecomment-619322526 https://api.github.com/repos/pydata/xarray/issues/4002 MDEyOklzc3VlQ29tbWVudDYxOTMyMjUyNg== max-sixty 5635139 2020-04-25T05:10:56Z 2020-04-25T05:10:56Z MEMBER

OK, I think that's fairly rare, but depending on @shoyer 's preferences, changing those to a check and raise seems reasonable to me

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove dangerous default argument 606549469
619307939 https://github.com/pydata/xarray/issues/4002#issuecomment-619307939 https://api.github.com/repos/pydata/xarray/issues/4002 MDEyOklzc3VlQ29tbWVudDYxOTMwNzkzOQ== shoyer 1217238 2020-04-25T02:39:10Z 2020-04-25T02:39:10Z MEMBER

These are appropriate uses for assert (verifying internal invariants). Please keep them.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove dangerous default argument 606549469
619297407 https://github.com/pydata/xarray/issues/4002#issuecomment-619297407 https://api.github.com/repos/pydata/xarray/issues/4002 MDEyOklzc3VlQ29tbWVudDYxOTI5NzQwNw== max-sixty 5635139 2020-04-25T01:17:34Z 2020-04-25T01:17:34Z MEMBER

I might be behind the curve on the pythonic approach there: what's wrong with asserts there?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove dangerous default argument 606549469
619270916 https://github.com/pydata/xarray/issues/4002#issuecomment-619270916 https://api.github.com/repos/pydata/xarray/issues/4002 MDEyOklzc3VlQ29tbWVudDYxOTI3MDkxNg== shoyer 1217238 2020-04-24T22:52:29Z 2020-04-24T22:52:29Z MEMBER

We would take a PR to change these to default None and then x=x or {}

I like this pattern a little better, because it's clear it's comparing to a default value: python if x is None: x = {}

 
{
    "total_count": 2,
    "+1": 2,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove dangerous default argument 606549469
619265079 https://github.com/pydata/xarray/issues/4002#issuecomment-619265079 https://api.github.com/repos/pydata/xarray/issues/4002 MDEyOklzc3VlQ29tbWVudDYxOTI2NTA3OQ== max-sixty 5635139 2020-04-24T22:28:52Z 2020-04-24T22:28:52Z MEMBER

We would take a PR to change these to default None and then x=x or {}

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove dangerous default argument 606549469
619248867 https://github.com/pydata/xarray/issues/4002#issuecomment-619248867 https://api.github.com/repos/pydata/xarray/issues/4002 MDEyOklzc3VlQ29tbWVudDYxOTI0ODg2Nw== shoyer 1217238 2020-04-24T21:35:33Z 2020-04-24T21:35:33Z MEMBER

Yes, this would be nice to clean up, but mostly just for the sake of writing idiomatic Python. We don't ever modify any of these arguments.

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Remove dangerous default argument 606549469

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