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

handle variable timezone offsets in dates due daylight saving #86

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

abalazsik
Copy link
Contributor

Hi! I've noticed that in the JSON responses of my server the timezones are off by 1 hour in the summer months. After digging a little bit, I found out that this library assumes that all timezone offsets are constant during the year. But it is not true in many countries.
I'm in the Europe/Budapest timezone. We use the UTC + 1 during winter, and UTC + 2 timezone during the summer due the daylight saving procedure.

Here is a little fix that calculates the current zone offset based on the current time. I implemented it as a new datetime_format so it will not break any existing code.

@sile
Copy link
Owner

sile commented Sep 15, 2024

Thank you for your PR.
I have a question: is there a reason why the {iso8601, N} when N :: integer() option cannot be used for your purpose?

@abalazsik
Copy link
Contributor Author

Yes, It can be (In fact, I do this as a quick fix). But I thought I it would be better to make a fix for it, since I expected that the {iso8601, local} should handle this

@sile
Copy link
Owner

sile commented Sep 16, 2024

I understand the reason. Thank you.
This PR seems almost okay, but could you please update the doc and spec about datetime_format at https://github.com/sile/jsone/blob/master/src/jsone.erl#L161?

@abalazsik
Copy link
Contributor Author

Sure, done

Copy link
Owner

@sile sile left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@sile sile merged commit 8c97402 into sile:master Sep 16, 2024
@sile
Copy link
Owner

sile commented Sep 17, 2024

Hi @abalazsik, I have another question.

The local_dst timezone value is calculated using the current time and remains fixed during an encoding call.
So, is it correct to use this fixed timezone value when encoding a datetime value that is (for example) several months in the past?
In other words, is it unnecessary to calculate an individual timezone value for each datetime value?

@abalazsik
Copy link
Contributor Author

abalazsik commented Sep 17, 2024

Hmm... I'm not sure. I think this is how it works
drawing

@sile
Copy link
Owner

sile commented Sep 17, 2024

Hmm, I'm not familiar with daylight saving, but the following behavior feels strange:

%% Call the function during the summer
> jsone:encode({{2024, 9, 18}, {3,0,0}}, [{datetime_format,{iso8601,local_dst}}]).
<<"\"2024-09-18T03:00:00+02:00\"">>

%% Call the function during the winter 
%% => The input value remains the same, but the output differs
> jsone:encode({{2024, 9, 18}, {3,0,0}}, [{datetime_format,{iso8601,local_dst}}]).
<<"\"2024-09-18T03:00:00+01:00\"">>

I think that if there is a time adjustment between T1 and T2, it is not surprising that the results of T2 - T1 and K2 - K1 differ. Perhaps only K2 - K1 should be used to calculate the actual time difference.

However, if the current behavior is not causing any trouble to you, it is okay to keep the current local_dst behavior.
(i.e., local_dst is merely a shorthand for the current timezone calculation and does not take the input values into account)

@abalazsik
Copy link
Contributor Author

The wikipedia article about ISO 8601 says this:

"The standard provides a well-defined, unambiguous method of representing calendar dates and times in worldwide communications, especially to avoid misinterpreting numeric dates and times when such data is transferred between countries with different conventions for writing numeric dates and times"

It is an extra data for the consumer, so it can transform the date to its timezone. It does not make sense to send the timezone of timestamp value. The example might feel strange, but it shows that the timezone calculation is not a "pure function", it depends on the current timezone.

@sile
Copy link
Owner

sile commented Sep 17, 2024

I see. Thank you.

@abalazsik
Copy link
Contributor Author

Anytime :-)

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 this pull request may close these issues.

2 participants