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

Switch smaller TimeSpan constants to int from long #103721

Closed
wants to merge 2 commits into from

Conversation

IDisposable
Copy link
Contributor

Allow more use cases where unintended overflows are unlikely.

Allow more use cases where unintended overflows are unlikely.
@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.

@tannergooding
Copy link
Member

I think we need to get the final status figured out before merging this or revert the original PR until that's done. Otherwise, we risk shipping things in a state that can't be rectified later.

There's both pros and cons to shipping these as int or shipping them as long. There's some arguments for ease of use with common values for int and there's some arguments for overall consistency for long.

There's likewise concerns about potential silent failures that can be introduced by using int for units where TimeSpan allows them to be long.

@IDisposable
Copy link
Contributor Author

IDisposable commented Jun 19, 2024

@tannergooding I will just hold waiting for the final decision. I agree there are pro/con in each direction. That said, my personal preference was for all long because that's more likely to avoid accidental overflows and guide folks towards intentional cast truncation when they've recognized the issue.

@tarekgh
Copy link
Member

tarekgh commented Jun 19, 2024

I think we need to get the final status figured out before merging this or revert the original PR until that's done. Otherwise, we risk shipping things in a state that can't be rectified later.

I don't think this issue is going to be resolved in a few hours and we get the final changes merged. I'll resurrect the revert PR again then.

@IDisposable sorry for the inconvenience. We'll keep you informed about the decision. Properly we'll need to close this PR too.

@tarekgh
Copy link
Member

tarekgh commented Jun 19, 2024

@tannergooding could you please approve the revert #103704?

@tarekgh
Copy link
Member

tarekgh commented Jun 19, 2024

Closing this for now. I have marked the issue #94545 as ready for review again.

@IDisposable
Copy link
Contributor Author

We've settled on long for everything except the HoursPerDay constant. See final PR #103993

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants