-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Add nullable annotations to Healthchecks #22785
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
Conversation
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
@@ -8,6 +8,6 @@ public sealed class HealthCheckContext | |||
/// <summary> | |||
/// Gets or sets the <see cref="HealthCheckRegistration"/> of the currently executing <see cref="IHealthCheck"/>. | |||
/// </summary> | |||
public HealthCheckRegistration Registration { get; set; } | |||
public HealthCheckRegistration Registration { get; set; } = default!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this only nullable for testing? Should add comment why it has been forced to non-nullable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the calling code assumes this is non-null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -243,7 +243,7 @@ public async Task CheckHealthAsync_SetsRegistrationForEachCheck() | |||
public async Task CheckHealthAsync_Cancellation_CanPropagate() | |||
{ | |||
// Arrange | |||
var insideCheck = new TaskCompletionSource<object>(); | |||
var insideCheck = new TaskCompletionSource<object?>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non generic TaskCompletionSource is merged in runtime. We should have it soon 🙏
Hello @pranavkm! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer bringing these to API review?
@BrennanConroy the suggestion was to review these together rather than individually. I'll set up a separate review once we've had more of these projects updated. |
No description provided.