Migrations: Annotate NRTs #24311
Migrations: Annotate NRTs #24311
Conversation
test/EFCore.SqlServer.FunctionalTests/Migrations/SqlServerMigrationsSqlGeneratorTest.cs
Show resolved
Hide resolved
| @@ -1635,10 +1639,10 @@ public MigrationBuilder([CanBeNull] string activeProvider) | |||
| public virtual OperationBuilder<UpdateDataOperation> UpdateData( | |||
| [NotNull] string table, | |||
| [NotNull] string[] keyColumns, | |||
| [NotNull] object[] keyValues, | |||
| [NotNull] object?[] keyValues, | |||
There was a problem hiding this comment.
Not sure, do we allow nulls in key values?
There was a problem hiding this comment.
🤷♂️ @AndriySvyryd?
| @@ -17,25 +19,25 @@ public class AddCheckConstraintOperation : MigrationOperation, ITableMigrationOp | |||
| /// <summary> | |||
| /// The name of the check constraint. | |||
| /// </summary> | |||
| public virtual string Name { get; [param: NotNull] set; } | |||
| public virtual string Name { get; [param: NotNull] init; } = null!; | |||
There was a problem hiding this comment.
Looking at usage, it seems like we could add a constructor that accepts the non-nullable types - instantiation sites already initialize these properties (but with an initializer instead of a constructor). Would be a (very) small breaking change, but may be worth it?
(same for the other operations)
There was a problem hiding this comment.
I refuse to make breaking changes for the sake of nullability (although I'll certainly avoid several patterns in the future because of it). But I'm not against adding the constructors now and putting [Obsolete] on the default constructor... We should discuss in a design meeting.
There was a problem hiding this comment.
But I will push back against anything that's part of an object graph since constructors force you to build everything inside-out which is much too tedious in tests.
P.S. This init was accidentally left in--I was playing with different patterns trying to get the compiler to warn if you didn't initialize the properties during object initialization. Unfortunately, it would require a custom analyzer.
There was a problem hiding this comment.
It was just a suggestion, definitely nothing we have to do. Migration operations do seem to me like simple-enough types that this pattern would well for them.
P.S. This init was accidentally left in--I was playing with different patterns trying to get the compiler to warn if you didn't initialize the properties during object initialization. Unfortunately, it would require a custom analyzer.
I think that by design, init-only properties aren't meant to be "these must be set during initialization", only "these may be set during initialization, but not afterwards".
src/EFCore.Relational/Migrations/Operations/InsertDataOperation.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Migrations/Operations/UpdateDataOperation.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Outdated
Show resolved
Hide resolved
test/EFCore.SqlServer.FunctionalTests/Migrations/SqlServerMigrationsSqlGeneratorTest.cs
Show resolved
Hide resolved
Fixes dotnet#24328, part of dotnet#19007
|
Hello @bricelam! 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 (
|
|
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
ghost
merged commit 
Fixes #24328, part of #19007