Add nullable annotations to Microsoft.AspNetCore.Mvc.Abstractions #22993
Conversation
pranavkm
force-pushed
the
prkrishn/nullable-mvc
branch
from
30cfbc3 to
860cd1d
Compare
|
Hello @pranavkm! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 60 minutes, a condition that will be fulfilled in about 54 minutes. No worries though, I will be back when the time is right! 😉 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 (
|
pranavkm
force-pushed
the
prkrishn/nullable-mvc
branch
from
860cd1d to
53357e9
Compare
Pilchie
added
the
area-mvc
|
|
||
| /// <summary> | ||
| /// The set of parameters associated with this action. | ||
| /// </summary> | ||
| public IList<ParameterDescriptor> Parameters { get; set; } | ||
| public IList<ParameterDescriptor> Parameters { get; set; } = Array.Empty<ParameterDescriptor>(); |
There was a problem hiding this comment.
These are not read-only, if we do this we can cause issues to existing code when they call Add
There was a problem hiding this comment.
These get initialized later on by the framework. It's not an issue in actual usage.
There was a problem hiding this comment.
Sure, I don't really understand why we prefer Array.Empty<T> to default! but I don't feel super strongly about it.
| actionContext?.HttpContext!, | ||
| actionContext?.RouteData!, | ||
| actionContext?.ActionDescriptor!, | ||
| actionContext?.ModelState!) | ||
| { |
There was a problem hiding this comment.
I'm missing something, how can actionContext be null here if you are requiring it not to be null? Why do we need the ? operators and the !?
There was a problem hiding this comment.
could we do actionContext! instead here?
There was a problem hiding this comment.
for the other constructor we should remove the nullchecks and use the ! in the parameter instead.
There was a problem hiding this comment.
You're right, i'll change the paramater to be nullable. There are unit test code paths this gets used in where the context is null
| @@ -1,6 +1,8 @@ | |||
| // Copyright (c) .NET Foundation. All rights reserved. | |||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | |||
|
|
|||
| #nullable enable | |||
There was a problem hiding this comment.
Can we get rid of the usages of this class in favor of the one in the framework?
| @@ -89,7 +89,7 @@ public class CompositeBindingSource : BindingSource | |||
| /// <inheritdoc /> | |||
| public override bool CanAcceptDataFrom(BindingSource bindingSource) | |||
| { | |||
| if (bindingSource == null) | |||
| if (bindingSource is null) | |||
There was a problem hiding this comment.
It would help if you keep focus on this already big PRs. Small changes like this distract and don't add value individually. (If we decided to convert the entire repo to use this style then so be it, but there's no agreement on this, and it would be more efficient done in a single pass).
There was a problem hiding this comment.
The compiler was complaining because the type has an equality operator. LHS is a non-null type, and RHS is null, so it says the two can never be equal. It was weird, but I didn't want to spend too much time on this
pranavkm
force-pushed
the
prkrishn/nullable-mvc
branch
from
53357e9 to
4f17721
Compare
|
🆙 📅 |
pranavkm
force-pushed
the
prkrishn/nullable-mvc
branch
from
4f17721 to
4ec628a
Compare
ghost
merged commit 
Contributes to #5680