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 optional custom error messages for *all* validation parameter attributes (not just ValidateScript and ValidatePattern) #3745

Open
mklement0 opened this Issue May 9, 2017 · 10 comments

Comments

Projects
None yet
3 participants
@mklement0
Contributor

mklement0 commented May 9, 2017

This is a follow-up to #3630.
Related: #3765

Thanks to @powercode's efforts in #2728, , we now have an optional ErrorMessage property in the ValidatePattern and ValidateScript parameter validation attributes. However, all validation attributes (Validation*; all that derive from System.Automation.Management.ValidateArgumentsAttribute) should support a custom error message:

  • Even for the simplest validations it can sometimes be helpful to provide domain-specific guidance rather than reporting a purely technical violation (see example below).

  • Consistent support eliminates the burden of having to remember which specific attributes support the property (though IntelliSense may ease that pain).

Example

function foo {
  param(
  [ValidateCount(2, [int]::maxvalue, ErrorMessage='This bar has a 2-drink minimum.')]
  [string] $bar
  )
  $bar
}

foo -bar 'mint julep'

Desired behavior

foo : Cannot validate argument on parameter 'bar'. This bar has a 2-drink minimum.
...

As in the implementation for ValidatePattern and ValidateScript, placeholder {0} would represent the value passed by the user, and {1}, ... the validation-attribute arguments.

@lzybkr

This comment has been minimized.

Show comment
Hide comment
@lzybkr

lzybkr May 10, 2017

Member

This idea was discussed and rejected in that PR, e.g. #2728 (comment)

Member

lzybkr commented May 10, 2017

This idea was discussed and rejected in that PR, e.g. #2728 (comment)

@mklement0

This comment has been minimized.

Show comment
Hide comment
@mklement0

mklement0 May 10, 2017

Contributor

@lzybkr:

(Your link label suggests that it links to a specific comment in the thread, but it doesn't.)

Reading through that thread, without understanding all the subtleties, I see a potential rejection argument based on technical inconvenience:

I think your suggestion was proposed and I'm not in favor of it because it provides a false promise - a validation attribute may ignore the property.

After further thought - there might be a way for the engine to check if an error message uses the ErrorMessage property in the exception's message - and if it doesn't - replace the error.

I think this might be reasonable to implement, but it feels a little clunky. Because Exception.Message is read-only - we'd need to create a new exception, likely using the original one as the InnerException.

Is this what you're referring to?

Either way, for future reference, could you please summarize the reason for rejecting this proposal here, given that it makes the case for all validation attributes supporting a custom error message in a more focused fashion?

Aside from what subset of validation attributes should support custom error messages: what are your thoughts on supporting localization? Should I create a separate issue?

Contributor

mklement0 commented May 10, 2017

@lzybkr:

(Your link label suggests that it links to a specific comment in the thread, but it doesn't.)

Reading through that thread, without understanding all the subtleties, I see a potential rejection argument based on technical inconvenience:

I think your suggestion was proposed and I'm not in favor of it because it provides a false promise - a validation attribute may ignore the property.

After further thought - there might be a way for the engine to check if an error message uses the ErrorMessage property in the exception's message - and if it doesn't - replace the error.

I think this might be reasonable to implement, but it feels a little clunky. Because Exception.Message is read-only - we'd need to create a new exception, likely using the original one as the InnerException.

Is this what you're referring to?

Either way, for future reference, could you please summarize the reason for rejecting this proposal here, given that it makes the case for all validation attributes supporting a custom error message in a more focused fashion?

Aside from what subset of validation attributes should support custom error messages: what are your thoughts on supporting localization? Should I create a separate issue?

@iSazonov

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov May 10, 2017

Collaborator

I still think we should be more common here with custom message.

@mklement0 I believe that we should not mix "custom message" and "message localization". Localizing messages for scripts looks a bit excessive complication.

Collaborator

iSazonov commented May 10, 2017

I still think we should be more common here with custom message.

@mklement0 I believe that we should not mix "custom message" and "message localization". Localizing messages for scripts looks a bit excessive complication.

@lzybkr

This comment has been minimized.

Show comment
Hide comment
@lzybkr

lzybkr May 10, 2017

Member

If ErrorMessage is available to all validation attributes, we are forced to choose between:

  • Inconsistent support - not every implementation of the attribute will utilize the message
  • Consistent support via hacky means - basically the engine would be forced to do unnatural things to ensure the message is used

Neither option is ideal. Existing validation attributes that know nothing about this new property may not be updated, and script authors may not test their use of ErrorMessage, so they may not realize it's at best a comment.

On the other hand, if the PowerShell engine is forced to wrap an exception when it detects the ErrorMessage is not used, the engine is making a choice to trust the user of the attribute over the author of the attribute. This might be a reasonable assumption, but it might not. But worse, to use the custom error message, PowerShell would be forced to create a new exception, and whenever we wrap exceptions, we are creating an inconsistent experience because sometimes you need to unwrap (when ErrorMessage is used) and sometimes you don't.

Member

lzybkr commented May 10, 2017

If ErrorMessage is available to all validation attributes, we are forced to choose between:

  • Inconsistent support - not every implementation of the attribute will utilize the message
  • Consistent support via hacky means - basically the engine would be forced to do unnatural things to ensure the message is used

Neither option is ideal. Existing validation attributes that know nothing about this new property may not be updated, and script authors may not test their use of ErrorMessage, so they may not realize it's at best a comment.

On the other hand, if the PowerShell engine is forced to wrap an exception when it detects the ErrorMessage is not used, the engine is making a choice to trust the user of the attribute over the author of the attribute. This might be a reasonable assumption, but it might not. But worse, to use the custom error message, PowerShell would be forced to create a new exception, and whenever we wrap exceptions, we are creating an inconsistent experience because sometimes you need to unwrap (when ErrorMessage is used) and sometimes you don't.

@lzybkr

This comment has been minimized.

Show comment
Hide comment
@lzybkr

lzybkr May 10, 2017

Member

Also, I do think localization is a distinct issue.

Member

lzybkr commented May 10, 2017

Also, I do think localization is a distinct issue.

@mklement0

This comment has been minimized.

Show comment
Hide comment
@mklement0

mklement0 May 11, 2017

Contributor

Re localization: Makes sense - please see #3765.

@iSazonov: I know that localization is a feature not used by many, but we should support it comprehensively.

Contributor

mklement0 commented May 11, 2017

Re localization: Makes sense - please see #3765.

@iSazonov: I know that localization is a feature not used by many, but we should support it comprehensively.

@mklement0 mklement0 changed the title from Implement optional custom error messages for *all* validation parameter attributes (not just ValidateScript and ValidatePattern) and support localized messages to Implement optional custom error messages for *all* validation parameter attributes (not just ValidateScript and ValidatePattern) May 11, 2017

@mklement0

This comment has been minimized.

Show comment
Hide comment
@mklement0

mklement0 May 11, 2017

Contributor

@lzybkr: Thanks for the summary.

Consistent support via hacky means - basically the engine would be forced to do unnatural things to ensure the message is used

I can see how that's undesirable.

Inconsistent support - not every implementation of the attribute will utilize the message

Are you referring to third-party implementations of attributes?

Simply ensuring that all validation attributes that ship with PowerShell support custom error messages, combined with documenting the recommendation that third-party implementers respect them too seems like a reasonable compromise to me.

Without the hack, users are already left guessing: namely, as to what subset of the shipping-with-PowerShell attributes happen to implement custom error messages, and what governed membership in that subset.

Contributor

mklement0 commented May 11, 2017

@lzybkr: Thanks for the summary.

Consistent support via hacky means - basically the engine would be forced to do unnatural things to ensure the message is used

I can see how that's undesirable.

Inconsistent support - not every implementation of the attribute will utilize the message

Are you referring to third-party implementations of attributes?

Simply ensuring that all validation attributes that ship with PowerShell support custom error messages, combined with documenting the recommendation that third-party implementers respect them too seems like a reasonable compromise to me.

Without the hack, users are already left guessing: namely, as to what subset of the shipping-with-PowerShell attributes happen to implement custom error messages, and what governed membership in that subset.

@lzybkr

This comment has been minimized.

Show comment
Hide comment
@lzybkr

lzybkr May 11, 2017

Member

Yes, third party implementations, and I suppose future first-party implementations where somebody forgets about this property.

I believe it's the engine's responsibility to provide good error messages everywhere, and only when that's not possible should we allow customization.

Another example (copied because GitHub can't seem to link correctly) :

With a custom error message, it's possible for a cmdlet author to provide a worse error message than the default, especially without localized messages.

For a few of the attributes, I can't convince myself it's worth it. And there are minor costs - an extra pointer that is never used isn't free - it requires more memory and extra time in garbage collection checking if the (usually null) value needs collecting.

ValidateNotNullOrEmpty stands out as one example where I wouldn't bother. ValidateDrive is also probably not worth it.

And last - I can see a custom error message being useful on ValidateRange, e.g. to say the value should be a positive value or whatever, but if we go that route, we need localization support similar to HelpMessageBaseName and HelpMessageResourceId in ParameterAttribute.

So using the ValidateRange example, I would rather we add ValidatePositive and ValidateNonNegative attributes so that nobody has to worry about localization and is not tempted to use ValidateRange with the ErrorMessage property.

As for guessing - Intellisense does a good job completing property names in attributes, so I don't think it's a big problem.

Member

lzybkr commented May 11, 2017

Yes, third party implementations, and I suppose future first-party implementations where somebody forgets about this property.

I believe it's the engine's responsibility to provide good error messages everywhere, and only when that's not possible should we allow customization.

Another example (copied because GitHub can't seem to link correctly) :

With a custom error message, it's possible for a cmdlet author to provide a worse error message than the default, especially without localized messages.

For a few of the attributes, I can't convince myself it's worth it. And there are minor costs - an extra pointer that is never used isn't free - it requires more memory and extra time in garbage collection checking if the (usually null) value needs collecting.

ValidateNotNullOrEmpty stands out as one example where I wouldn't bother. ValidateDrive is also probably not worth it.

And last - I can see a custom error message being useful on ValidateRange, e.g. to say the value should be a positive value or whatever, but if we go that route, we need localization support similar to HelpMessageBaseName and HelpMessageResourceId in ParameterAttribute.

So using the ValidateRange example, I would rather we add ValidatePositive and ValidateNonNegative attributes so that nobody has to worry about localization and is not tempted to use ValidateRange with the ErrorMessage property.

As for guessing - Intellisense does a good job completing property names in attributes, so I don't think it's a big problem.

@mklement0

This comment has been minimized.

Show comment
Hide comment
@mklement0

mklement0 May 11, 2017

Contributor

Thanks for that. Looking at the set of built-in attributes more closely, I now see how there's not much benefit in allowing custom messages for the remaining ones (doing so would also be complicated by several attributes reporting one of several messages).

In that vein:

  • I hadn't noticed that ValidateSetAttribute was also given an ErrorMessage property - it seems like a prime candidate for not needing that.

  • Conversely,

    • ValidateLength deserves an analogous makeover to ValidateCount's (#3585):

      • distinguish only 2 cases: min. and max. length being equal, and being outside the range
      • the current messages are quite pleonastic (I'm in the mood for big words) and prescriptive; e.g.: The character length of the 3 argument is too long. Shorten the character length of the argument so it is fewer than or equal to "2" characters, and then try the command again
    • Similarly, ValidateRange should probably have only a single (built-in) outside-the-range error message (a min == max case doesn't make sense here).


nobody has to worry about localization

Given that we'll have (at least) 2 attributes that do support custom error messages - ValidateScriptAttribute and ValidatePatternAttribute - isn't the localization can of worms / gusanos / Würmer / vers already open?

P.S.: The link to the specific comment in the other thread worked this time.

Contributor

mklement0 commented May 11, 2017

Thanks for that. Looking at the set of built-in attributes more closely, I now see how there's not much benefit in allowing custom messages for the remaining ones (doing so would also be complicated by several attributes reporting one of several messages).

In that vein:

  • I hadn't noticed that ValidateSetAttribute was also given an ErrorMessage property - it seems like a prime candidate for not needing that.

  • Conversely,

    • ValidateLength deserves an analogous makeover to ValidateCount's (#3585):

      • distinguish only 2 cases: min. and max. length being equal, and being outside the range
      • the current messages are quite pleonastic (I'm in the mood for big words) and prescriptive; e.g.: The character length of the 3 argument is too long. Shorten the character length of the argument so it is fewer than or equal to "2" characters, and then try the command again
    • Similarly, ValidateRange should probably have only a single (built-in) outside-the-range error message (a min == max case doesn't make sense here).


nobody has to worry about localization

Given that we'll have (at least) 2 attributes that do support custom error messages - ValidateScriptAttribute and ValidatePatternAttribute - isn't the localization can of worms / gusanos / Würmer / vers already open?

P.S.: The link to the specific comment in the other thread worked this time.

@lzybkr

This comment has been minimized.

Show comment
Hide comment
@lzybkr

lzybkr May 12, 2017

Member

I approved the custom message on ValidateSet because sometimes the set is so large that a custom message might be better.

For completeness, we should support localization of the custom error message, but I didn't raise that issue because:

  1. we don't currently have any localization outside of official Windows PowerShell
  2. a custom error message is very likely better than any localized message for the generic message for the attributes where we allow the custom error message
Member

lzybkr commented May 12, 2017

I approved the custom message on ValidateSet because sometimes the set is so large that a custom message might be better.

For completeness, we should support localization of the custom error message, but I didn't raise that issue because:

  1. we don't currently have any localization outside of official Windows PowerShell
  2. a custom error message is very likely better than any localized message for the generic message for the attributes where we allow the custom error message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment