Skip to content
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

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

AndriySvyryd
Copy link
Member

Rename GetDiscriminatorProperty to FindDiscriminatorProperty

Part of #19213

@AndriySvyryd AndriySvyryd requested a review from a team February 18, 2021 02:02
@@ -41,6 +46,104 @@ public IEnumerable<IKey> GetContainingKeys()
public IClrPropertyGetter GetGetter()
=> throw new NotImplementedException();

public IComparer<IUpdateEntry> GetCurrentValueComparer()
Copy link
Member

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

Copy link
Member Author

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(
Copy link
Member

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)

Copy link
Member Author

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")]
Copy link
Member

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?

Copy link
Member Author

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.

src/EFCore/Metadata/Internal/IInternalModel.cs Outdated Show resolved Hide resolved
Rename GetDiscriminatorProperty to FindDiscriminatorProperty

Part of #19213
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants