fixed bug in tz_localize to throw an error if the argument to the ambiguous parameter is not one of the 4 correct options #49856
Conversation
There was a problem hiding this comment.
Given #49565 (comment), I do believe the original issue is behaving as expected and a fix isn't needed here.
If you are looking for issues to tackle, I would recommend searching for issues that have good first issue and avoid issues that still have need triage
|
closing as per #49565 (comment), but thanks anyway for the pR |
floor_timestamp_tests.py
Outdated
| import pandas as pd | ||
| import unittest | ||
|
|
||
| class MyTestCase(unittest.TestCase): |
There was a problem hiding this comment.
this isn't the right location for tests, can you search for similar tests within the pandas test suite and add it there?
Also, you can use pytest.mark.parametrize to parametrize over arguments, no need to write 6 different tests
There was a problem hiding this comment.
this comment still needs addressing
There was a problem hiding this comment.
You can disregard floor_timestamp_tests.py. The test cases in pandas/tests/scalar/timestamp/test_timezones.py cover the bug already and they are passing on my end
pandas/_libs/tslibs/timestamps.pyx
Outdated
| if ambiguous == 'infer': | ||
| raise ValueError('Cannot infer offset with only one time.') | ||
|
|
||
| if ambiguous != True and ambiguous != False and ambiguous != 'NaT' and ambiguous != 'raise': |
There was a problem hiding this comment.
let's write this as
if not isinstance(ambiguous, bool) and ambiguous not in {'NaT', 'raise'}:
floor_timestamp_tests.py
Outdated
| import pandas as pd | ||
| import unittest | ||
|
|
||
| class MyTestCase(unittest.TestCase): |
There was a problem hiding this comment.
this comment still needs addressing
|
I believe the test cases were failing after I merged with the main branch to keep it up to date, but I fixed the issue and they should be passing now |
| tz = warsaw | ||
| tz = "Europe/Warsaw" |
There was a problem hiding this comment.
why does this need to change?
There was a problem hiding this comment.
I didn't change that so I'm not sure, it changed when I merged with the main branch
There was a problem hiding this comment.
looks like something went wrong when merging, please try again
There was a problem hiding this comment.
It looks like there were some changes to these files on the pandas-dev main branch, I'll sync with my branch to get it up to date first
There was a problem hiding this comment.
Ok, so I merged with with the pandas/dev main branch, then re-added my changed with tests and pre-commit passing. I accidentally closed this while merging so if you can't re-open, the new pull request is here: #50024
| with pytest.raises(pytz.AmbiguousTimeError, match=msg): | ||
| ts.tz_localize("dateutil/US/Central") | ||
|
|
||
| if ZoneInfo is not None: | ||
| try: | ||
| tz = ZoneInfo("US/Central") | ||
| except KeyError: | ||
| # no tzdata | ||
| pass | ||
| else: | ||
| with pytest.raises(pytz.AmbiguousTimeError, match=msg): | ||
| ts.tz_localize(tz) | ||
|
|
There was a problem hiding this comment.
This also got deleted when I merged with the main branch
flooring,ambiguousparameter unnecessarily used (and raising Error) #49565doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.