Skip to content

DEPR: parsing to tzlocal #50949

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Feb 1, 2023
Merged

DEPR: parsing to tzlocal #50949

merged 6 commits into from
Feb 1, 2023

Conversation

jbrockmendel
Copy link
Member

@mroeschke mroeschke added the Deprecate Functionality to remove in pandas label Jan 24, 2023
mroeschke
mroeschke reviewed Jan 26, 2023
View reviewed changes
pandas/tests/tslibs/test_array_to_datetime.py
@@ -67,6 +67,7 @@ def test_parsing_timezone_offsets(dt_string, expected_tz):
assert result_tz == timezone(timedelta(minutes=expected_tz))


@pytest.mark.filterwarnings("ignore:.*dependent on system timezone.*:FutureWarning")
def test_parsing_non_iso_timezone_offset():
dt_string = "01-01-2013T00:00:00.000000000+0000"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't a UTC offset like this be okay?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why but this triggered the tzlocal warning on one the builds. best guess is that build had the system timezone set to gmt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I thought the spirit of the original deprecation is to forbid the 3 letter abbreviated timezones?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the spirit is to not give results that depend on your system timezone. so if there is a system-config that results in getting tzlocal in this case, that user will in the future get the same non-tzlocal tzinfo that everyone else gets. i.e. this is not going to start raising for anybody.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay that makes sense. I am just concerned about the seemingly false positive for this test case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. im open to suggestions on how to avoid it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like many CI runs have time.tzname == ("UTC", "UTC"), end up getting tzlocal instead of a tzutc(). Updated to avoid warning in that case.

@jbrockmendel
Copy link
Member Author

i think issues have been addressed.

MarcoGorelli
MarcoGorelli approved these changes Feb 1, 2023
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely - system-dependent behaviour is really hard to debug

@MarcoGorelli MarcoGorelli added this to the 2.0 milestone Feb 1, 2023
@mroeschke mroeschke merged commit 38e802c into pandas-dev:main Feb 1, 2023
@mroeschke
Copy link
Member

Nice thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the depr-tzlocal branch February 1, 2023 19:31
@mroeschke
Copy link
Member

I think this needed a rebase? Seeing this on main

     def test_parsing_tzlocal_deprecated():
        # GH#50791
        msg = "Pass the 'tz' keyword or call tz_localize after construction instead"
        dtstr = "Jan 15 2004 03:00 EST"
    
        with tm.set_timezone("US/Eastern"):
            with tm.assert_produces_warning(FutureWarning, match=msg):
                res, _ = parse_datetime_string_with_reso(dtstr)
    
            assert isinstance(res.tzinfo, tzlocal)
    
            with tm.assert_produces_warning(FutureWarning, match=msg):
>               res = parsing.parse_datetime_string(dtstr)
E               AttributeError: module 'pandas._libs.tslibs.parsing' has no attribute 'parse_datetime_string'

@jbrockmendel
Copy link
Member Author

Yah, we'll need to change that to check py_parse_datetime_string

@MarcoGorelli
Copy link
Member

have opened a pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroeschke mroeschke mroeschke approved these changes

@MarcoGorelli MarcoGorelli MarcoGorelli approved these changes

Assignees
No one assigned
Labels
Deprecate Functionality to remove in pandas
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

DEPR: parsing tzlocal depending on user's system timezone
3 participants
@jbrockmendel @mroeschke @MarcoGorelli