Annotate the remainder of metadata for nullability#23905
Conversation
roji
force-pushed
the
MetadataNullability4
branch
from
72263d3 to
5b64e1a
Compare
| public static IConventionDbFunctionBuilder HasDbFunction( | ||
| [NotNull] this IConventionModelBuilder modelBuilder, | ||
| [NotNull] string name, | ||
| [CanBeNull] string? name, |
There was a problem hiding this comment.
This is called from RelationalEntityTypeBuilderExtensions.ToFunction with a null name. Should we add another HasDbFunction on RelationalModelBuilderExtensions which doesn't accept a returnType, and accepts a nullable name? Or something else?
There was a problem hiding this comment.
It just shouldn't be called if name is null, it will throw
There was a problem hiding this comment.
Sorry if I'm misunderstanding, but I don't see where RelationalEntityTypeBuilderExtensions.ToFunction would throw before calling this... Did you want me to add a check and throw somewhere, or just skip the HasDbFunction call (and so just do SetFunctionName(null) on the entity type?
There was a problem hiding this comment.
I mean if HasDbFunction is called with null name then Check.NotEmpty will throw, so the HasDbFunction call should be skipped
There was a problem hiding this comment.
OK, adding a condition in ToFunction around the HasDbFunction call to skip it when name is null (so it only performs SetFunctionName)
…MappingConvention.cs Co-authored-by: Andriy Svyryd <AndriySvyryd@users.noreply.github.com>
|
It merged without waiting for checks to complete 😮 |
|
Yeah... Just posted on the team channel. I guess we don't have a rule that requires CI checks to be green... |
By design. Hopefully the auto-complete feature is not limited to the same rules for manual merges, otherwise I doubt we can use it. |
|
😥 welp, at least the build happened to pass, looks like I'm a pretty lucky guy... Yes, AFAIK the feature simply applies the rules that have been defined. If we want to be able to manually merge even though the CI isn't passing, I guess we'll have to disable this. |
|
Then it doesn't replace the bot, just offers a different way of merging |
|
If we can't use it before CI is done, I'd personally just use the bot... |
|
We could require green again, but we know that there are times when C.I. flakiness makes that difficult. It's up to the team. |
4 participants
Sorry @AndriySvyryd :(
There's a lot of stuff here, I did my best - I know there are some bangs that could be investigated further. I suggest concentrating on the public APIs - anything on private/internal can be easily improved later.
Note that for the various Builders, the vast majority of accesses assume they aren't null (i.e. not removed from the model). So I made them non-nullable, and added a separate IsInModel property for checking that. Did similar with ConventionDispatcher and the recently-introduced IsReadonly.
Part of #19007