Many of the [Xyz]Resource types in this namespace throw an Exception if an attempt is made to read a non-nullable property when there's no value for that property in the underlying data.
As per the following comment from @idg10, this is likely incorrect and should be reviewed across the Marain.UserNotifications.Client project.
Exception is almost never the right type to throw. It should be something more specific. But I'm wondering if this is even the correct place to report this error.
Is there a reason not to check for this and throw it in the constructor? If it's illegal to have an SmeTemplateResource that doesn't include a notification type, we shouldn't really let people construct such a thing, and the ctor should throw an ArgumentException indicating that if someone tries.
If on the other hand we do in fact permit it, then it's not exactly a "schema violation". It's more just one possible state. And if we are saying "It's OK not to have a notification type, it's just that you mustn't ask what the notification type is when it's not there" then this should be an InvalidOperationException, and the error text should say something like "The NotificationType property must not be used on a template that does not specify a NotificationType".
Or this could return string?.
I don't know which of these options is correct, but what we have right now is wrong on two fronts: uninformative exception type, and a message that either blames the caller for a mistake that was actually made somewhere else (during construction), or it blames them for the wrong thing (attempting to use a property on an object that happens not to have that property available).
Originally posted by @idg10 in #132 (comment)
Many of the [Xyz]Resource types in this namespace throw an
Exceptionif an attempt is made to read a non-nullable property when there's no value for that property in the underlying data.As per the following comment from @idg10, this is likely incorrect and should be reviewed across the Marain.UserNotifications.Client project.
Exceptionis almost never the right type to throw. It should be something more specific. But I'm wondering if this is even the correct place to report this error.Is there a reason not to check for this and throw it in the constructor? If it's illegal to have an
SmeTemplateResourcethat doesn't include a notification type, we shouldn't really let people construct such a thing, and the ctor should throw anArgumentExceptionindicating that if someone tries.If on the other hand we do in fact permit it, then it's not exactly a "schema violation". It's more just one possible state. And if we are saying "It's OK not to have a notification type, it's just that you mustn't ask what the notification type is when it's not there" then this should be an
InvalidOperationException, and the error text should say something like "The NotificationType property must not be used on a template that does not specify a NotificationType".Or this could return
string?.I don't know which of these options is correct, but what we have right now is wrong on two fronts: uninformative exception type, and a message that either blames the caller for a mistake that was actually made somewhere else (during construction), or it blames them for the wrong thing (attempting to use a property on an object that happens not to have that property available).
Originally posted by @idg10 in #132 (comment)