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

Feat: calendar:(universal/local)_time_to_system_time/1,2 #9445

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MarkoMin
Copy link
Contributor

@MarkoMin MarkoMin commented Feb 15, 2025

Added missing conversions.

Added proper tests. I've tried them locally with proper

I really hope it's not too late to get it into OTP 28...

Closes #8160

Copy link
Contributor

github-actions bot commented Feb 15, 2025

CT Test Results

    2 files     97 suites   1h 8m 33s ⏱️
2 192 tests 2 145 ✅ 47 💤 0 ❌
2 558 runs  2 509 ✅ 49 💤 0 ❌

Results for commit 9b34810.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@MarkoMin MarkoMin force-pushed the feat/calendar/to_system_time branch from 7d1f670 to b5ead3d Compare February 15, 2025 11:11
@IngelaAndin IngelaAndin added the team:VM Assigned to OTP team VM label Feb 17, 2025
@MarkoMin MarkoMin force-pushed the feat/calendar/to_system_time branch from b5ead3d to eadf47e Compare February 17, 2025 11:02
@MarkoMin
Copy link
Contributor Author

Resolved and squashed

@jhogberg jhogberg self-assigned this Feb 17, 2025
@jhogberg jhogberg added the testing currently being tested, tag is used by OTP internal CI label Feb 19, 2025
Copy link
Contributor

@jhogberg jhogberg left a comment

Choose a reason for hiding this comment

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

Thanks, the tests ran fine. Can you update the since versions as indicated? I'll merge it for inclusion in RC2 as soon as that's done. :-)


-doc(#{since => <<"OTP 28.0">>}).
-doc """
Converts local time into system time.
Copy link
Contributor

@garazdawi garazdawi Feb 21, 2025

Choose a reason for hiding this comment

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

Suggested change
Converts local time into system time.
Converts local time into [system time](`e:erts:time_correction.md#os-system-time`).

Add a link to system time definition and a new-line to only make the above part of the function summary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or is it POSIX Time that we are converting to... I always get all these time variants mixed up...

Copy link
Contributor Author

@MarkoMin MarkoMin Feb 21, 2025

Choose a reason for hiding this comment

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

I think that all functions in this module assume POSIX time... You can pass it any timestamp as an input and the conversion doesn't depend on OS time, right? I think that link to OS system time would create confusion

Co-authored-by: John Högberg <john@erlang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing conversions in calendar
4 participants