-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Record type follow ups: #25218
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
Record type follow ups: #25218
Conversation
* Throw an error if a record type property has validation metadata * Disallow TryUpdateModel on a top-level record type * Ignore previously specified model value when binding a record type * Unskip record type tests * Clean up record type detection
0682f1b
to
3e8a947
Compare
@@ -137,37 +139,16 @@ internal IReadOnlyList<ModelMetadata> BoundProperties | |||
} | |||
} | |||
|
|||
/// <summary> | |||
/// A mapping from parameters to their corresponding properties on a record type. | |||
/// </summary> |
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.
Nice to have doc comments for internal
API given the complexities in this area❕
src/Mvc/test/Mvc.IntegrationTests/ValidationWithRecordIntegrationTests.cs
Outdated
Show resolved
Hide resolved
src/Mvc/test/Mvc.IntegrationTests/ValidationWithRecordIntegrationTests.cs
Show resolved
Hide resolved
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.
Looks good to me!
Haven't looked at the tests too closely, but the code changes look good.
Co-authored-by: James Newton-King <[email protected]>
Co-authored-by: Doug Bunting <[email protected]>
Co-authored-by: Doug Bunting <[email protected]>
src/Mvc/test/Mvc.IntegrationTests/ValidationWithRecordIntegrationTests.cs
Outdated
Show resolved
Hide resolved
src/Mvc/test/Mvc.IntegrationTests/ValidationWithRecordIntegrationTests.cs
Outdated
Show resolved
Hide resolved
src/Mvc/test/Mvc.IntegrationTests/ValidationWithRecordIntegrationTests.cs
Outdated
Show resolved
Hide resolved
src/Mvc/test/Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs
Outdated
Show resolved
Hide resolved
@Pilchie could you merge this PR? |
Approved for RC1. |
Fixes #24265