CLN: remove unused entries from _lite_rule_alias #56516
Conversation
natmokval
changed the title
pandas/_libs/tslibs/offsets.pyx
Outdated
| "min", | ||
| "ns", | ||
| "us", |
There was a problem hiding this comment.
can you remove unrelated changes please?
There was a problem hiding this comment.
If we remove these from _dont_uppercase the tests test_offset_freqstr() and test_rule_code() in pandas/tests/tseries/offsets/test_offsets.py will fail. The reason: we uppercase names of offsets class in _get_offset(). In PR #56346 where we deprecate uppercasing/lowercasing we will remove them from _dont_uppercase
There was a problem hiding this comment.
ah, thanks - so, they're not currently unused then? but will become unused once #56346 is enforced? should this PR wait until after that one then?
There was a problem hiding this comment.
I checked in PR #56346, we should keep "min", "us", "ns" in the list _dont_uppercase to pass these tests.
On the other hand, we have all lowercase offsets frequencies in this list ('h', 's', etc.), and it looks more clear then have these entries ("min": "min", "us": "us", etc.) in dict _lite_rule_alias.
There was a problem hiding this comment.
thanks for explaining
sorry for not having got round to this yet, was quite this week with a client project - but I'll get to it, thanks for looking at it all so carefully!
There was a problem hiding this comment.
thank you for helping me with my PRs!
MarcoGorelli
changed the title
| freq="US", | ||
| freq="us", |
There was a problem hiding this comment.
does this PR make to_offset('US') suddently start failing? if so, I don't think we can do this yet (at least, not before https://github.com/pandas-dev/pandas/pull/56346/files ?)
There was a problem hiding this comment.
does this PR make
to_offset('US')suddently start failing? if so, I don't think we can do this yet (at least, not before https://github.com/pandas-dev/pandas/pull/56346/files ?)
yes, this test didn't fail for uppercase frequency "US" before this PR.
There was a problem hiding this comment.
a small correction to the comment above:
we fixed the test_partial_slice_second_precision in PR DEPR: lowercase strings denoting freq for Week, MonthBegin, MonthEnd, etc. #56346
but so far on main we have to_offset('US') in this test and it pass.
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
|
I tried to do something similar in #59240 - fancy taking a look please? |
yeah, sure, I'll take a look. |
|
I got the same idea, to move aliases "us", and"ns" from seems like all tests passes except some tests in |
|
closed as completed via #59240 |
xref #56346
remove unused entries
"min", "ms", "us", "ns"from _lite_rule_alias