-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Move IProperty and IPropertyBase extension methods to the interfaces #24180
Conversation
@@ -41,6 +46,104 @@ public IEnumerable<IKey> GetContainingKeys() | |||
public IClrPropertyGetter GetGetter() | |||
=> throw new NotImplementedException(); | |||
|
|||
public IComparer<IUpdateEntry> GetCurrentValueComparer() |
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.
nit: all these can be expression-bodied (on same line) to cut down on lines :) Also in other test files
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.
I'll let code cleanup handle this, VS can't apply this formatting in the whole file
/// <param name="propertyAccessMode"> The <see cref="PropertyAccessMode" />, or null to clear the mode set.</param> | ||
/// <param name="fromDataAnnotation"> Indicates whether the configuration was specified using a data annotation. </param> | ||
/// <returns> The configured value. </returns> | ||
PropertyAccessMode? SetPropertyAccessMode( |
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.
Any reason for these not to have default interface implementations, moving the code up from PropertyBase? This seems to be possible in many cases (i.e. where annotations are being looked up)
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.
I wanted to be consistent in the way annotations are used - a given annotation should only be used in the default implementations or not used there at all. Since having only some methods using it would create a leaky abstraction that could potentially cause bugs if we decide to move from annotation to field implementation one or more of the classes that implements the interface.
@@ -14,29 +12,8 @@ namespace Microsoft.EntityFrameworkCore | |||
/// <summary> | |||
/// Extension methods for <see cref="IConventionPropertyBase" />. | |||
/// </summary> | |||
[Obsolete("Use IConventionPropertyBase")] |
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.
If we think it's important to maintain binary compat (not sure it is), we can keep the extension methods, and simply have them call the newly-introduced interface methods.
Otherwise is there any value in keeping empty extensions classes with obsolete attributes on them?
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.
This is to help users that call these methods using the non-extension syntax (we've had quite a few calls of this type in our code). Also if binary compat issues are reported we can selectively restore those methods.
Rename GetDiscriminatorProperty to FindDiscriminatorProperty Part of #19213
662b996
to
74ddd03
Compare
Rename
GetDiscriminatorProperty
toFindDiscriminatorProperty
Part of #19213