Unclear logic for which offsets are _adjust_dst

[jbrockmendel]

The base class DateOffset has _adjust_dst = False. Then almost every subclass overrides it to True. The exceptions being: BusinessHour, CustomBusinessHour
LastWeekOfMonth (in contrast to WeekOfMonth, seems like a likely mistake)
Tick and subclasses.

The Tick classes make sense since those are glorified Timedeltas. BusinessHour I could imagine being special because it is sub-daily, but I'd appreciate a confirmation that this is intentional. LastWeekofMonth I suspect is a mistake.

I'll roll this into an existing issue and close once I get the lay of the land.

[rockg]

That was added in #9623. Basically some rules need to remove the timezone, apply the rule, and then add the timezone back in order to get the right offset after applying the rule. These are mostly rules that use dateutil.relativedelta for the underlying logic. Timedeltas just work out of the box around the DST transition so these subclasses did not need any adjustment. Not clear if LastWeekOfMonth just works.

[jbrockmendel]

@rockg thanks, that's a helpful reference. Unfortunately you've made the mistake of volunteering yourself for follow-up questions.

These are mostly rules that use dateutil.relativedelta for the underlying logic

Luckily we're down to the base DateOffset class being the only one that uses relativedeltas anymore. The singular/plural args for that have been the source of quite a bit of confusion.

Not clear if LastWeekOfMonth just works.

The sanity test I've been using recently is to check that (ts + offset) - offset == ts if and only if offset.onOffset(ts). The add-then-subtract implementation is the general version in the DateOffset class, so I'm implicitly assuming that it should hold forever and always (so if you have reason to believe this heuristic is wrong, please let me know). By that measure, LastWeekOfMonth definitely does not work.

As for BusinessHour and CustomBusinessHour I haven't looked at them that closely yet. If you happen to have any insight into the decision there, I'd appreciate it.

And last, there's a roundup in #18854 of offsets-related issues. A handful of them are along the lines of "if anybody knows what was intended here please let me know". If you find yourself with nothing better to do...

[rockg]

The singular/plural is confusing for relativedelta but if one knows that those are passed to dateutil, the dateutil documentation is pretty clear as to how those are handled.

That sanity check isn't clear that it will work properly over DST transitions. At least before this change was made, I believe that logic will work but the ts + offset will result in the wrong DST, but subtracting the offset will "undo" the incorrect DST. The tests I added in the PR explicitly test that the DST transitions are correct and that is the litmus test for these types of operations. BusinessHour and CusomBusinessHour seem to primarily use timedeltas which should just work. However, these, like LastWeekOfMonth weren't added to the tests so it's probably good to add them to see if they work.

It's not clear with the transition to the cython routines if the _adjust_dst flag is still needed for everything or not. Worth taking an offset class setting it to False and seeing if it works.

[rockg]

Also, the test you describe is another completely necessary test of offset operations.