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

time: update unix time acces, fix issues related to deviating unix times #21293

Merged
merged 9 commits into from
Apr 16, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Apr 16, 2024

The PR fixes issues like #17162 and #17311

The fix does NOT add the @[noinit] attribute to the Time struct, as doing so would break its use in other structs, such as:

struct Foo {
	t time.Time
}

foo := Foo{} // would not be possible anymore if Time is turned into a noinit struct
  • The problem is fixed by ensuing explicitness - while still keeping the possibility the to create Time structs.

    Only the unix field is turned into a private field. Not being able to init it with other fields resolves their interferes. Accessing and explicitly setting the unix field is possible via unix() function and method. This approach should provide a better predictable result and more general flexibility in comparison to a @[noinit] solution.

  • The next change is making sure that the unix time is present or calculated when a time is used.

  • Unix methods have been updated in the go spirit. Former methods have been deprecatd.

    `unix_time` -> `unix`
    `unix_time_milli` -> `unix_milli`
    

While initially planned making it into a v fmt autofix, types of selector expressions are not parsed and not accessible via vfmt. Therefore I'm making the access of a .unix file into a checker warning, informing that it'll become an error and recommending to use unix() instead.

ref. https://discord.com/channels/592103645835821068/592320321995014154/1229503437029380107

@ttytm ttytm added the Breaking Change This PR introduces changes that break backward compatibility. Requires manual review. label Apr 16, 2024
@ttytm
Copy link
Member Author

ttytm commented Apr 16, 2024

@JalonSolov it was a bit suboptimal with the last PR. Please go ahead here 👍

Comment on lines -105 to +136
return t.unix
return t.unix()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should still work without the () since it's in the same module... correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, correct

Copy link
Member Author

@ttytm ttytm Apr 16, 2024

Choose a reason for hiding this comment

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

But we can keep it here then it calls the function to ensure a unix time is present or calculated if it isn't. So we don't need to wrap it in new() / new_time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense, then, to change the other t.unix to t.unix()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Where we can't be sure if an expression has a unix time, this definitively makes sense.

I hope that I applied it now everywhere where it is required to ensure robustness for all cases and only left where internal direct access makes total sense (e.g. when using an internal variable has a now() time value - which certainly will have the unix time on it).

@medvednikov medvednikov merged commit 1363cc8 into vlang:master Apr 16, 2024
56 checks passed
@ttytm ttytm deleted the time/fix branch April 16, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change This PR introduces changes that break backward compatibility. Requires manual review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants