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

Remove timezone support from sas_numeric_to_date() #583

Closed
billdenney opened this issue Nov 18, 2024 · 3 comments · Fixed by #584
Closed

Remove timezone support from sas_numeric_to_date() #583

billdenney opened this issue Nov 18, 2024 · 3 comments · Fixed by #584

Comments

@billdenney
Copy link
Collaborator

While working on #582, there was an issue with timezone tests in sas_numeric_to_date().

Looking at https://communities.sas.com/t5/Ask-the-Expert/Tips-and-Tricks-for-Working-with-Dates-and-Times-in-SAS-Q-amp-A/ta-p/813974, it appears that SAS likely (but not definitively) doesn't support storing time zones and it likely stores the timezone as the current time zone. The documentation that I can find is not clear.

Therefore, I plan to remove timezone support from the function and always use UTC timezones. That seems like the least-bad choice because a SAS file sent to a computer in a timezone that has different daylight saving time method would likely get different results, based on my reading.

@billdenney
Copy link
Collaborator Author

The implementation in #584 doesn't remove timezone support but gives a warning when it's used with any timezone other than UTC.

@sfirke
Copy link
Owner

sfirke commented Nov 19, 2024

The implementation in #584 doesn't remove timezone support but gives a warning when it's used with any timezone other than UTC.

I'm reviewing #584 and want to make sure I understand:

  1. is the idea that the input tz value is ignored and UTC used for the conversion, with the only use of the existing tz argument being to warn users who have legacy code using that argument?

  2. If that's right, does that mean this will be a breaking change for those users because the same input will now result in a warning and a different result?

@billdenney
Copy link
Collaborator Author

  1. tz is not ignored. Now UTC is used by default but the user can specify a different tz value.
  2. It is a breaking change that tz used to be "" by default and now it's "UTC". And, yes, there is a warning which is technically a breaking change (but the warning doesn't change the calculation results).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants