-
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
API review updates #31673
API review updates #31673
Conversation
ajcvickers
commented
Sep 9, 2023
- Rename ConverterMappingHints.Override to WithOverride
- Rename the RelationalTypeMapping.Clone methods to sue With
- Rename ILoggingOptions.ShouldWarnForEnumType to ShouldWarnForStringEnumValueInJson
- Rename parameter to enumType
- Rename IClrPropertyGetter/Setter methods
- Make IReadOnlyProperty.GetElementType return IReadOnlyElementType
- Make NullTypeMapping pubternal
- Remove the SQLite HasSrid extension methods from primitive collection builders
- Remove IInternalEntry
- Use ITypeBase for ParameterBindingInfo constructor
/// <param name="fromDataAnnotation">Indicates whether the configuration was specified using a data annotation.</param> | ||
/// <returns>The configuration for the elements.</returns> | ||
IElementType? ElementType(bool primitiveCollection, bool fromDataAnnotation = false); | ||
IConventionElementType? SetElementType(bool elementType, bool fromDataAnnotation = false); |
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'm not convinced this is the best name. SetIsPrimitiveCollection
makes more sense to me.
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 plan to work on another PR today that allows an element type that is not the type of a primitive collection. This allows things like ranges to be mapped with an element type.
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.
/cc @roji
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.
Even so, this refers to the existence of an element type, not the element type itself.
SetHasElementType
?
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.
@AndriySvyryd See #31677
3020dbf
to
9fe6ba6
Compare
- Rename ConverterMappingHints.Override to WithOverride - Rename the RelationalTypeMapping.Clone methods to sue With - Rename ILoggingOptions.ShouldWarnForEnumType to ShouldWarnForStringEnumValueInJson - Rename parameter to enumType - Rename IClrPropertyGetter/Setter methods - Make IReadOnlyProperty.GetElementType return IReadOnlyElementType - Make NullTypeMapping pubternal - Remove the SQLite HasSrid extension methods from primitive collection builders - Remove IInternalEntry - Use ITypeBase for ParameterBindingInfo constructor
9fe6ba6
to
25c3bd1
Compare
Any changes in here (or other change between RC1 and RC2) that are breaking, we need to add to the efcore breaking change doc because RC1 was a go-live release. The change in RelationalTypeMapping seems to be API breaking, eg.. |
@danmoseley in an offline chat I summarized the different approach we have to customer-facing breaking changes and provider-facing ones, let's maybe have a chat about this if you want. |
Ah interesting yes would like to learn, we can chat offline. |