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

Add a initializer to Google_Protobuf_Timestamp with rounding control #1748

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

thomasvl
Copy link
Collaborator

Provide a new initializer with explicit rounding controls and map the other initializer though the single code paths.

Deprecate the old apis to direct folks to the new one to acknowledge the rounding that is happening on their values.

Provide a new initializer with explicit rounding controls and map the
other initializer though the single code paths.

Deprecate the old apis to direct folks to the new one to acknowledge the
rounding that is happening on their values.
/// - imeIntervalSince1970: The `TimeInterval`, interpreted as
/// seconds relative to 00:00:00 UTC on 1 January 1970.
/// - rule: The rounding rule to use.
public init(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@allevato and I were discussion options here, and this seems to follow things like Date in that it includes time interval in the name. Other things we considered:

init(
    roundingSince1970 timeInterval: TimeInterval,
    rule: FloatingPointRoundingRule
)

Which provides a slightly shorter name than what is suggested here.

init(
    timeIntervalSince1970: TimeInterval,
    roundingRule: FloatingPointRoundingRule
)

Would mean we don't have to deprecate the current method for the default rounding, but the naming of the rule argument is slightly different than foundation apis.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@FranzBusch @tbkka @Lukasa any thoughts on the naming choices here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do quite like the new name, honestly.

@thomasvl thomasvl added the 🆕 semver/minor Adds new public API. label Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants