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

Introducing Time abstraction Part1 #83604

Merged
merged 7 commits into from
Mar 25, 2023
Merged

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Mar 17, 2023

#36617

This change is adding the time abstraction to the .NET 8.0. Next part is to support that for down-levels.

@dotnet-issue-labeler
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, to 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.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@tarekgh tarekgh requested a review from stephentoub March 17, 2023 17:55
@ghost ghost assigned tarekgh Mar 17, 2023
@tarekgh tarekgh added this to the 8.0.0 milestone Mar 17, 2023
@stephentoub
Copy link
Member

stephentoub commented Mar 17, 2023

Can you call out any places you made changes from the commit I gave you?

I also don't see any tests?... nevermind, they were hiding in the diff.

@tarekgh
Copy link
Member Author

tarekgh commented Mar 17, 2023

@stephentoub I marked the places I changed as much as I can remember. Will be good to skim over the whole changes just in case spot something else.

@stephentoub
Copy link
Member

I marked the places I changed as much as I can remember. Will be good to skim over the whole changes just in case spot something else.

Thanks. FWIW, in such cases in the future you can start from the commit and just make changes in a new one. If you want to retrofit it, you can stash your changes, cherry-pick the commit, and then apply the changes on top of what's there (or a simple version of that just copy/paste your files on top of the ones from the cherry-pick). Regardless, knowing the diffs now will help me review, thanks.

/// each time it's called. That capture can be suppressed with <see cref="ExecutionContext.SuppressFlow"/>.
/// </para>
/// </remarks>
public abstract ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period);
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to consider some kind of out parameter that would allow this to communicate back whether all work has quiesced? Or are you thinking we'll investigate that as a follow-up? This is to allow, for example, CancellationTokenSource.TryReset to work with other timer implementations.

Copy link
Member Author

@tarekgh tarekgh Mar 23, 2023

Choose a reason for hiding this comment

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

Let's have a follow up on that to see if there are any more things we need to consider. I'll create an issue for that.

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.

7 participants