home / github

Menu
  • GraphQL API
  • Search all tables

issue_comments

Table actions
  • GraphQL API for issue_comments

7 rows where author_association = "MEMBER" and issue = 683954433 sorted by updated_at descending

✎ View and edit SQL

This data as json, CSV (advanced)

Suggested facets: created_at (date), updated_at (date)

user 5

  • Illviljan 3
  • shoyer 1
  • max-sixty 1
  • mathause 1
  • keewis 1

issue 1

  • Should __repr__ and __str__ be PEP8 compliant? · 7 ✖

author_association 1

  • MEMBER · 7 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
733989093 https://github.com/pydata/xarray/issues/4367#issuecomment-733989093 https://api.github.com/repos/pydata/xarray/issues/4367 MDEyOklzc3VlQ29tbWVudDczMzk4OTA5Mw== keewis 14808389 2020-11-25T23:25:44Z 2020-11-25T23:25:44Z MEMBER

This seems to be resolved so I'll close this issue. Feel free to post / reopen if you disagree.

Just for reference, both pytest and the standard library doctest runner allow running setup code: python @pytest.fixture(autouse=True) def custom_function(doctest_namespace): # the name of the function does not matter # add setup code here ... The official way to call the builtin doctest runner seems to be to add a if __name__ == "__main__" with a call to doctest.testmod() and to then call the module using either python <filename>.py or python -m <filename>. All code before the call to testmod can be used to set up the environment of the doctest.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Should __repr__ and __str__ be PEP8 compliant? 683954433
678738626 https://github.com/pydata/xarray/issues/4367#issuecomment-678738626 https://api.github.com/repos/pydata/xarray/issues/4367 MDEyOklzc3VlQ29tbWVudDY3ODczODYyNg== Illviljan 14371165 2020-08-23T07:10:26Z 2020-08-23T07:10:26Z MEMBER

Pandas breaks PEP8 (80 > 79) too then and Numpy is PEP8 compliant to one indent level (75 + 4) which I think is fine.

I see no point in temporarily changing settings as it will not work with doctest. I will work around it by printing numpy arrays instead.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Should __repr__ and __str__ be PEP8 compliant? 683954433
678722063 https://github.com/pydata/xarray/issues/4367#issuecomment-678722063 https://api.github.com/repos/pydata/xarray/issues/4367 MDEyOklzc3VlQ29tbWVudDY3ODcyMjA2Mw== shoyer 1217238 2020-08-23T02:54:04Z 2020-08-23T02:54:04Z MEMBER

Our default line width limit for text repr() is actually already 80 characters. This matches display.width from pandas, and is a little longer than what NumPy uses (75 characters).

You can definitely set a lower line width if you like for rendering repr() when copying and pasting into docstrings, using xarray.set_options.

I don't think we should change this overall, because copying and pasting into docstring is a relatively rare use of repr() compared to printing data in a console.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Should __repr__ and __str__ be PEP8 compliant? 683954433
678703013 https://github.com/pydata/xarray/issues/4367#issuecomment-678703013 https://api.github.com/repos/pydata/xarray/issues/4367 MDEyOklzc3VlQ29tbWVudDY3ODcwMzAxMw== mathause 10194086 2020-08-22T22:29:31Z 2020-08-22T22:29:31Z MEMBER

We ignore this error here:

https://github.com/pydata/xarray/blob/2bc8e33b319d54f9a6e89a88ac3161f4fb569fcf/setup.cfg#L109-L110

I think the argument is that a line lenght of 88 is much less likely to be too short: https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Should __repr__ and __str__ be PEP8 compliant? 683954433
678700741 https://github.com/pydata/xarray/issues/4367#issuecomment-678700741 https://api.github.com/repos/pydata/xarray/issues/4367 MDEyOklzc3VlQ29tbWVudDY3ODcwMDc0MQ== Illviljan 14371165 2020-08-22T22:06:26Z 2020-08-22T22:06:26Z MEMBER

Black defaults to 88 characters per line, which happens to be 10% over 80. This number was found to produce significantly shorter files than sticking with 80 (the most popular), or even 79 (used by the standard library). https://black.readthedocs.io/en/stable/the_black_code_style.html

I wonder why they don't reduce the indent size too if minimizing rows is of such importance.

The line width does make perfect sense if Black is the new de facto standard. I don't think it's the path of least resistance though. If 88 is used it will trigger other code style analyzers (it did for me) because they default to 79. PEP8 has been around for 19 years, black has a lot of work trying to persuade 19 years of people recommending following PEP8.

Have you updated the pep8speaks bot with this line length?

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Should __repr__ and __str__ be PEP8 compliant? 683954433
678677646 https://github.com/pydata/xarray/issues/4367#issuecomment-678677646 https://api.github.com/repos/pydata/xarray/issues/4367 MDEyOklzc3VlQ29tbWVudDY3ODY3NzY0Ng== max-sixty 5635139 2020-08-22T18:50:04Z 2020-08-22T18:50:04Z MEMBER

Thanks for the issue and the example.

Because we use black — and blacks increasing adoption in the python ecosystem — I think operation on its default 88 width is reasonable. Given the display width is currently 80, that gives us two tabs. I think that's probably OK. Are there cases where it doesn't work?

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Should __repr__ and __str__ be PEP8 compliant? 683954433
678619316 https://github.com/pydata/xarray/issues/4367#issuecomment-678619316 https://api.github.com/repos/pydata/xarray/issues/4367 MDEyOklzc3VlQ29tbWVudDY3ODYxOTMxNg== Illviljan 14371165 2020-08-22T09:35:12Z 2020-08-22T09:39:14Z MEMBER

Seems the max width is defined in https://github.com/pydata/xarray/blob/43a2a4bdf3a492d89aae9f2c5b0867932ff51cef/xarray/core/options.py. Setting OPTIONS[DISPLAY_WIDTH] = 71 does the trick. Not sure where else this option is used though.

{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
  Should __repr__ and __str__ be PEP8 compliant? 683954433

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]);
Powered by Datasette · Queries took 13.29ms · About: xarray-datasette