Skip to content
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.

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

Deprecate tz_localize, let cast_timezone work on naive #6561

Closed
MarcoGorelli opened this issue Jan 30, 2023 · 7 comments · Fixed by #6649
Closed

Deprecate tz_localize, let cast_timezone work on naive #6561

MarcoGorelli opened this issue Jan 30, 2023 · 7 comments · Fixed by #6649
Labels
enhancement New feature or an improvement of an existing feature

Comments

@MarcoGorelli
Copy link
Collaborator

MarcoGorelli commented Jan 30, 2023

the two behaviours I'm suggesting to deprecate are:
calling with_time_zone on tz-naive;
calling with_time_zone(None)

EDIT: this issue was originally about making with_time_zone stricter The scope then evolved into cast_time_zone and tz_localize. To keep the scope down, I've changed the scope of this suggestion to be only about cast_time_zone and tz_localize. I'll make a separate issue about with_time_zone

EDIT 2 I've broken the with_time_zone part of the suggestion off into #6658

@MarcoGorelli MarcoGorelli added the enhancement New feature or an improvement of an existing feature label Jan 30, 2023
@MarcoGorelli
Copy link
Collaborator Author

MarcoGorelli commented Feb 2, 2023

Right, having spent some more time on this, I think it could go further:

  • cast_time_zone would work on tz-naive, and do the same thing tz_localize currently does (I have something ready for this, it's relatively straightforward);
  • tz_localize would be deprecated (following @braaannigan's suggestion from a few weeks ago)
  • with_time_zone would only be used to convert from one tz-aware to another tz-aware (separate issue)

@ritchie46 @alexander-beedie @stinodego sorry for the ping, just wanted to check if you'd be on board with this


TLDR
There would only be 2 methods for time zones:

  • with_time_zone: convert timezone
  • cast_time_zone: set/unset/modify timezone

@FObersteiner
Copy link

uh I think @braaannigan in #6338 wanted to deprecate cast_time_zone, in favour of tz_localize ;-)

@MarcoGorelli
Copy link
Collaborator Author

true, but I think the spirit of the suggestion was that one method is redundant given the other two :)

I think it would be simpler for users if there were only two methods, and simpler for developers if cast_time_zone just had an extra arm in its match statement rather than having tz_localize just call cast_time_zone. I'll put up a PR to demonstrate, it might help

@MarcoGorelli MarcoGorelli changed the title Deprecate some flexibility in with_time_zone Deprecate tz_localize, let cast_timezone work on naive, stricter with_time_zone Feb 2, 2023
@MarcoGorelli MarcoGorelli changed the title Deprecate tz_localize, let cast_timezone work on naive, stricter with_time_zone Deprecate tz_localize, let cast_timezone work on naive Feb 3, 2023
@MarcoGorelli
Copy link
Collaborator Author

MarcoGorelli commented Feb 3, 2023

Update: I've opened a PR to let cast_time_zone work on tz-naive and thus deprecate tz_localize #6649

Letting cast_time_zone work on tz-naive and deprecating tz_localize, on the other hand, does feel quite natural. And users would just need to switch tz_localize with cast_time_zone and it would do the same thing, so it wouldn't be too breaking for them. So I went ahead with the PR - no hard feelings if you disagree with this suggestion of course!


Regarding making with_time_zone stricter: separate issue incoming, to keep the scope down

@FObersteiner
Copy link

letting with_time_zone accept None

what would be the result? does it revert to the internal representation / naive "UTC-like" ?

@MarcoGorelli
Copy link
Collaborator Author

MarcoGorelli commented Feb 3, 2023

that's right - it would keep doing what it currently does, there would be no change to with_time_zone (at least, not in this issue)

@FObersteiner
Copy link

@MarcoGorelli I really like the new function names as in 0.16.3 ! I'm still in favour of deprecating tz_localize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or an improvement of an existing feature
Projects
None yet
2 participants