issue_comments

Table actions

5 rows where issue = 374434077 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

  • Switch enable_cftimeindex to True by default · 5 ✖
id html_url issue_url node_id user created_at updated_at ▲ author_association body reactions performed_via_github_app issue
434899873 https://github.com/pydata/xarray/pull/2516#issuecomment-434899873 https://api.github.com/repos/pydata/xarray/issues/2516 MDEyOklzc3VlQ29tbWVudDQzNDg5OTg3Mw== shoyer 1217238 2018-11-01T01:22:01Z 2018-11-01T01:22:01Z MEMBER

In 6d08d3b I made changes to set_options to only fill the self.old dictionary with the old values for the keyword arguments specified in the constructor, and then had the exit method only reset the values that were temporarily set before. The win-32 build now passes, and we no longer have the undesirable warning behavior shown above. Do you think that was a reasonable fix?

This looks like a reasonable fix to me

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Switch enable_cftimeindex to True by default 374434077
434899060 https://github.com/pydata/xarray/pull/2516#issuecomment-434899060 https://api.github.com/repos/pydata/xarray/issues/2516 MDEyOklzc3VlQ29tbWVudDQzNDg5OTA2MA== spencerkclark 6628425 2018-11-01T01:17:06Z 2018-11-01T01:17:06Z MEMBER

Thanks @shoyer -- that was a bit puzzling but I think I sorted it out. I haven't done a careful diagnosis, but my hunch is that the previous build was running into this issue: https://github.com/pytest-dev/pytest/issues/2917. A fix was merged a few weeks ago, but the Python 2.7 32-bit build is/was using a relatively old version of pytest (3.5 versus the most recent 3.9.3).

In the previous build, the FutureWarning for setting the enable_cftimeindex option was being emitted earlier in test_backends.py in the test_open_mfdataset_manyfiles test (before we were watching for it in the options tests). It was being raised there, because upon calling set_options.__exit__, every option (not just the options that were set in a with statement) would be reset to its global value. For example doing something like this would emit the warning (which even outside of this minor testing issue, I think is undesirable): ``` In [1]: import xarray

In [2]: with xarray.set_options(display_width=40): ...: pass ...: /Users/spencerclark/xarray-dev/xarray/xarray/core/options.py:49: FutureWarning: The enable_cftimeindex option is now a no-op and will be removed in a future version of xarray. FutureWarning) `` In 6d08d3b I made changes toset_optionsto only fill theself.olddictionary with the old values for the keyword arguments specified in the constructor, and then had theexit` method only reset the values that were temporarily set before. The win-32 build now passes, and we no longer have the undesirable warning behavior shown above. Do you think that was a reasonable fix?

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Switch enable_cftimeindex to True by default 374434077
434747673 https://github.com/pydata/xarray/pull/2516#issuecomment-434747673 https://api.github.com/repos/pydata/xarray/issues/2516 MDEyOklzc3VlQ29tbWVudDQzNDc0NzY3Mw== shoyer 1217238 2018-10-31T16:08:46Z 2018-10-31T16:08:46Z MEMBER

I don't know why this failed in one of the Appveyor builds: ``` ================================== FAILURES =================================== _____ test_enable_cftimeindex _______ def test_enable_cftimeindex(): with pytest.raises(ValueError): xarray.set_options(enable_cftimeindex=None) with pytest.warns(FutureWarning, match='no-op'): with xarray.set_options(enable_cftimeindex=True):

          assert OPTIONS['enable_cftimeindex']

E Failed: DID NOT WARN. No warnings of type (<type 'exceptions.FutureWarning'>,) was emitted. The list of emitted warnings is: []. xarray\tests\test_options.py:38: Failed ```

 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Switch enable_cftimeindex to True by default 374434077
433446875 https://github.com/pydata/xarray/pull/2516#issuecomment-433446875 https://api.github.com/repos/pydata/xarray/issues/2516 MDEyOklzc3VlQ29tbWVudDQzMzQ0Njg3NQ== pep8speaks 24736507 2018-10-26T15:26:35Z 2018-10-27T18:13:36Z NONE

Hello @spencerkclark! Thanks for updating the PR.

Comment last updated on October 27, 2018 at 18:13 Hours UTC
 
{
    "total_count": 0,
    "+1": 0,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Switch enable_cftimeindex to True by default 374434077
433485272 https://github.com/pydata/xarray/pull/2516#issuecomment-433485272 https://api.github.com/repos/pydata/xarray/issues/2516 MDEyOklzc3VlQ29tbWVudDQzMzQ4NTI3Mg== dcherian 2448579 2018-10-26T17:32:55Z 2018-10-26T17:32:55Z MEMBER

Should we add nice error messages to resample and plot telling the user what to do if they try those two with cftimeindex?

 
{
    "total_count": 1,
    "+1": 1,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   Switch enable_cftimeindex to True by default 374434077

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