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

[Doc] Clarify description for get_unix_time_from_system on UTC #89454

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

iltenahmet
Copy link
Contributor

Change description of get_unix_time_from_system to clarify what it does regarding UTC.

Fixes this issue from the godot-docs repository.

@iltenahmet iltenahmet requested a review from a team as a code owner March 13, 2024 19:06
doc/classes/Time.xml Outdated Show resolved Hide resolved
doc/classes/Time.xml Outdated Show resolved Hide resolved
@AThousandShips AThousandShips added enhancement documentation cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 13, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Mar 13, 2024
@@ -180,7 +180,7 @@
<method name="get_unix_time_from_system" qualifiers="const">
<return type="float" />
<description>
Returns the current Unix timestamp in seconds based on the system time in UTC. This method is implemented by the operating system and always returns the time in UTC.
Returns the current Unix timestamp in seconds based on the system time. The Unix timestamp is the number of seconds passed since 00:00:00 UTC on 1 January 1970, the [url=https://en.wikipedia.org/wiki/Unix_time]Unix epoch[/url]. It is always in UTC.
Copy link
Member

@aaronfranke aaronfranke Mar 13, 2024

Choose a reason for hiding this comment

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

This is not correct. Timestamps do not need to be UTC (even though this method always is in UTC).

Most of the Time class works with "the same timezone", so you can give it a timestamp in any time zone. Saying that it is always in UTC is not consistent with most of the Time class which allows non-UTC timestamps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant that the Unix epoch is based on UTC (00:00:00 UTC on 1 January 1970), and since this method returns seconds passed since that time, I thought it would mean it's in UTC as well.

Should I just remove the last phrase "It is always in UTC." ?

Copy link
Member

@aaronfranke aaronfranke Mar 13, 2024

Choose a reason for hiding this comment

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

You can change it to this:

Returns the current Unix timestamp in seconds based on the system time in UTC. This method is implemented by the operating system and always returns the time in UTC. The Unix timestamp is the number of seconds passed since 1970-01-01 at 00:00:00, the [url=https://en.wikipedia.org/wiki/Unix_time]Unix epoch[/url].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the quick reply. I'm slightly confused about keeping "always returns the time in UTC" because it feels like we wouldn't be addressing the problem raised in the original issue.

This method returns a time interval. It doesn't make sense to state "returns the time in UTC". A time interval is independent of any time zone.

Sorry for the confusion.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your PR trying to solve this. There is some misunderstanding in the issue, I commented there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, thanks for the clarification there.

doc/classes/Time.xml Outdated Show resolved Hide resolved
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

I left one comment, and the commits should be squashed, and then this is a good improvement.

fix url tag in doc/classes/Time.xml

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>

fix url tag in doc/classes/Time.xml

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>

update get_unix_time_from_system description

Update doc/classes/Time.xml

Co-authored-by: Aaron Franke <arnfranke@yahoo.com>
@iltenahmet iltenahmet force-pushed the master branch 2 times, most recently from b1ae779 to 8bfc257 Compare March 13, 2024 23:47
@akien-mga akien-mga changed the title [Doc] Clarify description for get_unix_time_from_system on UTC [Doc] Clarify description for get_unix_time_from_system on UTC Mar 14, 2024
@akien-mga akien-mga merged commit 79d2ac3 into godotengine:master Mar 14, 2024
16 of 23 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@iltenahmet
Copy link
Contributor Author

🎉🎉 Thank y'all for being so welcoming and helpful.

@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 8, 2024
@akien-mga
Copy link
Member

Cherry-picked for 4.1.4.

@akien-mga akien-mga removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.
4 participants