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

Revert "Expose the useful constant values in TimeSpan" #103704

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Jun 19, 2024

Reverts #103498

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
This reverts commit 5fed175.
@dotnet-issue-labeler dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Jun 19, 2024
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@IDisposable
Copy link
Contributor

Did I break something?

@tarekgh
Copy link
Member Author

tarekgh commented Jun 19, 2024

@IDisposable I am temporarily reverting the change till we resolve the type of issue. I hurried merging the changes without addressing all comments. or we just implement the original approved proposal.

@tarekgh
Copy link
Member Author

tarekgh commented Jun 19, 2024

Did I break something?

No, not all reviewers agreed to have all constants as long. We need to have agreement on that before we have that in the product. I'll try to follow up.

@tarekgh tarekgh added area-System.DateTime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 19, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-datetime
See info in area-owners.md if you want to be subscribed.

@IDisposable
Copy link
Contributor

If we want to move most (non Tick*) to int, that's fine. I can make that change as desired. Just let me know and either reopen the PR or I'll start a new one.

@tarekgh
Copy link
Member Author

tarekgh commented Jun 19, 2024

If we want to move most (non Tick*) to int, that's fine. I can make that change as desired. Just let me know and either reopen the PR or I'll start a new one.

Can we go exact as the approved proposal? #94545 (comment).

Or you think int still can cause problems? if you are ok, we can close this revert PR and apply the changes over your previous changes in a new PR and we'll be fine. what do you think?

@IDisposable
Copy link
Contributor

I don't know of issues, I was left with the impression that avoiding potential of overflow was desired by this comment from @tannergooding... I welcome a final judgement.

Of course I could build it as proposed and see anything breaks in the tests.

@tarekgh
Copy link
Member Author

tarekgh commented Jun 19, 2024

Of course I could build it as proposed and see anything breaks in the tests.

Let's do that then. I'll close this PR. Are you going to submit a new PR? or I can do it if you are not interested?

@tarekgh
Copy link
Member Author

tarekgh commented Jun 19, 2024

We'll submit a change in another PR.

@tarekgh tarekgh closed this Jun 19, 2024
@tannergooding
Copy link
Member

Or you think int still can cause problems?

The main consideration from @bartonjs was that there are some constants, such as HoursPerDay which developers may want to use with int based scenarios (like int totalHours = days * TimeSpan.HoursPerDay).

Inversely, the concern I raised is that there are scenarios this can cause problems since days * TimeSpan.HoursPerDay could overflow. For example, if days > 89,478,485 then this will overflow to a negative int.

HoursPerDay happens to be the only case where it is "sensible" for it to be int, since both MaxDays (10,675,199) and MaxHours (256,204,778) are within int.MaxValue. That is also why the signatures are FromDays(int days, int hours, long minutes, long milliseconds, long microseconds). So other values, like MinutesPerDay (1440), MinutesPerHour (60), or even the existing MillisecondsPerTick (10,000) are all expected to be used in contexts where long is the target type.

@tarekgh
Copy link
Member Author

tarekgh commented Jun 19, 2024

@tannergooding let's apply the approved proposal first as the changes merged and we need to fix it before it goes out. Any other suggestion can be tracked to bring it again to the design review.

@IDisposable please let me know if you'll submit a new PR or want me to do it.

@bartonjs
Copy link
Member

They're also expected to be used in cases where int is the target type, and being long would be surprising. Sure, TimeSpan can go up to 10.6M days, but most people work on the order of 0..1000 days, and would be surprised that days * TimeSpan.SecondsPerDay is a long.

int totalSeconds = ...;
int totalDays = totalSeconds / TimeSpan.SecondsPerDay;

Nope. Doesn't compile.

@tannergooding
Copy link
Member

tannergooding commented Jun 19, 2024

Nope. Doesn't compile.

Yes, which is good because it is better for code to not compile than to silently fail due to an accidental overflow.

The user will get an error and they resolve the code by doing int totalDays = (int)(totalSeconds / TimeSpan.SecondsPerDay);. It may also make them aware of potential failures in the computation of totalSeconds since there are many scenarios under which it could have overflowed.

That is a stark contrast from the inverse case where int totalSeconds = x * TimeSpan.SecondsPerHour could trivially overflow and do so silently, for which the user likely won't find out until they hit a bug in production.

When considering TimeSpan, which is where these constants are defined, hours and days are int, all other units are long.

@IDisposable
Copy link
Contributor

I've put together the changes as originally approved and it builds on my machine.. I'll push a PR in a second.

@IDisposable
Copy link
Contributor

New PR to drop to int for most constants #103721

@tarekgh
Copy link
Member Author

tarekgh commented Jun 19, 2024

/ba-g this is straight reverted of other change. The failures are unrelated to the change.

@IDisposable
Copy link
Contributor

IDisposable commented Jun 25, 2024

After the revert, we've settled on exposing everything except HoursPerDay as a long.

New PR #103993

@tannergooding tannergooding deleted the revert-103498-expose-timespan branch June 25, 2024 20:55
@github-actions github-actions bot locked and limited conversation to collaborators Jul 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants