-
-
Notifications
You must be signed in to change notification settings - Fork 534
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
Use DisplayAttribute methods when getting name and description #1313
Conversation
`DisplayAttribute` has `GetName` and `GetDescription` methods that will return the `Name` and `Description` property values respectively if no `ResourceType` has been specified on the attribute, or will look up resource values if a `ResourceType` has been specified. This commit updates schema generation to use the above methods when a `DisplayAttribute` is used.
{ | ||
schema.Title = displayAttribute.Name; | ||
// GetName returns null if the Name property on the attribute is not specified. | ||
schema.Title = displayAttribute.GetName(); |
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.
Is using GetName() a breaking change from what we had before?
Maybe we should add a null check here as well (as before) as title might already be set and gets resetted to null otherwise.
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.
GetName() will return the value of the Name field if the ResourceType property on the DisplayAttribute is null, so in this circumstance, the behaviour is the same.
If a ResourceType property is specified on the DisplayAttribute, then GetName() will retrieve the value from the ResourceType instead e.g.
public class MyClass {
// No ResourceType specified: DisplayAttribute.GetName() returns "Some_Property"
[Display(Name = "Some_Property")]
public string SomeProperty { get; set; }
}
public class MyOtherClass {
// ResourceType specified: DisplayAttribute.GetName() returns MyResources.Some_Property
[Display(ResourceType = typeof(MyResources), Name = "Some_Property")]
public string SomeProperty { get; set; }
}
Current behaviour does not have a check to see if the title has already been set.
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.
Committed a check to ensure that the Name
property is not null before calling GetName()
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.
Instead of a .Name check and know the internals of GetName() I think it would be better to do:
if (displayAttribute != null)
{
var name = displayAttribute.GetName();
if (name != null)
{
schema.Title = name;
}
}
this way we only use GetName() and not assume a correlation between Name and GetName() (same for description)
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.
Okay, will do.
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.
Done: 3aa50c8
{ | ||
return displayAttribute.Description; | ||
// GetDescription returns null if the Description property on the attribute is not specified. | ||
var description = displayAttribute.GetDescription(); |
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.
Is using GetDescription() a breaking change from what we had before?
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.
GetDescription() will return the value of the Description field if the ResourceType property on the DisplayAttribute is null, so in this circumstance, the behaviour is the same.
If a ResourceType property is specified on the DisplayAttribute, then GetDescription() will retrieve the value from the ResourceType instead which is the intended behaviour of this change e.g.
public class MyClass {
// No ResourceType specified: DisplayAttribute.GetDescription() returns "Some_Property_Desc"
[Display(Description = "Some_Property_Desc")]
public string SomeProperty { get; set; }
}
public class MyOtherClass {
// ResourceType specified: DisplayAttribute.GetDescription() returns MyResources.Some_Property_Desc
[Display(ResourceType = typeof(MyResources), Name = "Some_Property_Desc")]
public string SomeProperty { get; set; }
}
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.
Committed a check to ensure that the Description
property is not null before calling GetDescription()
Only call DisplayAttribute's GetName() and GetDescription() methods if the respective Name and Description properties are non-null.
Thx lgtm |
Related to #1145.
DisplayAttribute
has GetName and GetDescription methods that will return theName
andDescription
property values respectively if noResourceType
has been specified on the attribute, or will look up resource values if aResourceType
has been specified.This PR updates schema generation to use the above methods when a
DisplayAttribute
is used, which allows localised resources to be used when generating schemas in addition to hard-coded name and description values.