Skip to content

CLN: unify a ValueError message for removed units T, L, U, N and remove these entries from UnitChoices #59119

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

Merged
merged 1 commit into from
Jun 27, 2024
+0 −22
Merged

CLN: unify a ValueError message for removed units T, L, U, N and remove these entries from UnitChoices #59119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions pandas/_libs/tslibs/dtypes.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,10 +453,6 @@ class Resolution(Enum):
"""
cdef:
str abbrev
if freq in {"T", "t", "L", "l", "U", "u", "N", "n"}:
raise ValueError(
f"Frequency \'{freq}\' is no longer supported."
)
try:
if freq in c_DEPR_ABBREVS:
abbrev = c_DEPR_ABBREVS[freq]
Expand Down
6 changes: 0 additions & 6 deletions pandas/_libs/tslibs/timedeltas.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ UnitChoices: TypeAlias = Literal[
"minute",
"min",
"minutes",
"T",
"t",
"s",
"seconds",
"sec",
Expand All @@ -50,21 +48,17 @@ UnitChoices: TypeAlias = Literal[
"millisecond",
"milli",
"millis",
"L",
"l",
"us",
"microseconds",
"microsecond",
"µs",
"micro",
"micros",
"u",
"ns",
"nanoseconds",
"nano",
"nanos",
"nanosecond",
"n",
]
_S = TypeVar("_S", bound=timedelta)

Expand Down
5 changes: 0 additions & 5 deletions pandas/_libs/tslibs/timedeltas.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1818,11 +1818,6 @@ class Timedelta(_Timedelta):
* 'microseconds', 'microsecond', 'micros', 'micro', or 'us'
* 'nanoseconds', 'nanosecond', 'nanos', 'nano', or 'ns'.

.. deprecated:: 2.2.0

Values `H`, `T`, `S`, `L`, `U`, and `N` are deprecated in favour
of the values `h`, `min`, `s`, `ms`, `us`, and `ns`.

.. deprecated:: 3.0.0

Allowing the values `w`, `d`, `MIN`, `MS`, `US` and `NS` to denote units
Expand Down
7 changes: 0 additions & 7 deletions pandas/tests/tslibs/test_resolution.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,3 @@ def test_units_H_S_deprecated_from_attrname_to_abbrevs(freq):

with tm.assert_produces_warning(FutureWarning, match=msg):
Resolution.get_reso_from_freqstr(freq)


@pytest.mark.parametrize("freq", ["T", "t", "L", "U", "N", "n"])
Copy link
Member

Choose a reason for hiding this comment

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

Are these units tested elsewhere (that they are disallowed)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, for these units we have tests:
test_period_index_T_L_U_N_raises in pandas/tests/indexes/period/test_constructors.py
test_frequency_H_T_S_L_U_N_raises in pandas/tests/indexes/datetimes/test_date_range.py
with the error message "Invalid frequency: {}”

and with another error message "invalid unit abbreviation: {unit}"
test_timedelta_unit_T_L_U_N_raises in pandas/tests/indexes/timedeltas/test_timedelta_range.py and
test_unit_T_L_N_U_raises pandas/tests/scalar/timedelta/test_constructors.py

I am not sure, should we unify this error message to "Invalid frequency: {}”?

I removed the test_reso_abbrev_T_L_U_N_raises from pandas/tests/tslibs/test_resolution.py because we get KeyError: 'T' for this test.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure, should we unify this error message to "Invalid frequency: {}”?

That would be nice, but if it makes the code more complex I would say it's not worth it

def test_reso_abbrev_T_L_U_N_raises(freq):
msg = f"Frequency '{freq}' is no longer supported."
with pytest.raises(ValueError, match=msg):
Resolution.get_reso_from_freqstr(freq)
Loading