-
Notifications
You must be signed in to change notification settings - Fork 473
[dev-v5] Extend DefaultValues with option to set default value on a base class #4999
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
base: dev-v5
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,13 @@ public void Set<TValue>(Expression<Func<TComponent, TValue>> parameterSelector, | |
| var propertyName = propertyInfo?.Name | ||
| ?? throw new ArgumentException($"The parameter selector '{parameterSelector}' does not resolve to a public property on the component '{typeof(TComponent)}'.", nameof(parameterSelector)); | ||
|
|
||
| if (!propertyInfo.CanWrite) | ||
| { | ||
| throw new ArgumentException($"The property '{propertyName}' on component '{typeof(TComponent)}' is read-only and cannot be used for default values.", nameof(parameterSelector)); | ||
| } | ||
|
|
||
| ValidateValueCompatibility(propertyInfo, value, propertyName); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to check the value's type? It takes time, especially when using reflection, and I don't really see the advantage: without this check, the code will crash anyway, right? Keep in mind that these methods are executed thousands of times since they're in the constructor of every component; they therefore need to be very efficie |
||
|
|
||
| // Add the value to the dictionary, using the property name as the key. | ||
| _values.AddOrUpdate(propertyName, AddFunction, UpdateFunction); | ||
|
|
||
|
|
@@ -68,4 +75,32 @@ public void Set<TValue>(Expression<Func<TComponent, TValue>> parameterSelector, | |
| return value; | ||
| } | ||
| } | ||
|
|
||
| private static void ValidateValueCompatibility(PropertyInfo propertyInfo, object? value, string propertyName) | ||
| { | ||
| var propertyType = propertyInfo.PropertyType; | ||
| var nullableUnderlyingType = Nullable.GetUnderlyingType(propertyType); | ||
| var targetType = nullableUnderlyingType ?? propertyType; | ||
|
|
||
| if (value is null) | ||
| { | ||
| if (propertyType.IsValueType && nullableUnderlyingType is null) | ||
| { | ||
| throw new ArgumentException($"Default value for '{propertyName}' on component '{typeof(TComponent)}' cannot be null because the property type '{propertyType}' is non-nullable.", nameof(value)); | ||
| } | ||
|
|
||
| var nullabilityInfo = new NullabilityInfoContext().Create(propertyInfo); | ||
| if (nullabilityInfo.WriteState == NullabilityState.NotNull) | ||
| { | ||
| throw new ArgumentException($"Default value for '{propertyName}' on component '{typeof(TComponent)}' cannot be null because the property is non-nullable.", nameof(value)); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
vnbaaij marked this conversation as resolved.
|
||
|
|
||
| if (!targetType.IsInstanceOfType(value)) | ||
| { | ||
| throw new ArgumentException($"Default value for '{propertyName}' on component '{typeof(TComponent)}' must be assignable to '{targetType}', but was '{value.GetType()}'.", nameof(value)); | ||
| } | ||
| } | ||
| } | ||
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.
There's going to be a performance issue here: you can't keep recalculating the BaseType parents all the time. You need to cache the type just once and then retrieve it without having to run through that loop again.
The logic must be different