-
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
Implement IsUnicode API #5890
Implement IsUnicode API #5890
Conversation
/// </summary> | ||
/// <param name="property"> The property to set the value for. </param> | ||
/// <param name="unicode"> True if the property accepts unicode characters, false if it does not, null to clear the setting. </param> | ||
public static void IsUnicode([NotNull] this IMutableProperty property, bool? unicode) |
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 it's first-class on the builder API, why not make it first-class on the Metadata API as well?
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.
Because we decided that for facets like this and max length we would make them annotations accessed by extension methods.
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 think we decided this mainly to keep the metadata interfaces minimal.
@rowanmiller Merged this, but would still appreciate you taking a look. |
Yep looks good to me |
.ValueGeneratedOnAdd(); | ||
|
||
b.Property<string>(""Name"") | ||
.HasAnnotation(""Unicode"", 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.
Should this be "lifted" to a Fluent API call? We do it for other relational configuration.
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.
@bricelam Good point. I'll do that.
Issue #3420
I think everything in the runtime and migrations areas is implemented. The ScaffoldingTypeMapper already supports unicode, but scaffolding will not make use of this until updated as described in issue #5410.
/cc @rowanmiller Can you look at the public API and doc comments?