Adding errormessage property to ValidatePatternAttribute #2728

powercode · 2016-11-19T02:07:28Z

Adding an ErrorMessage property to ValidatePatternAttribute so a message that
is meaningful to the user can be displayed, instead of a regular expression that
probably even the person who wrote will have a hard time understanding.

msftclas · 2016-11-19T02:07:32Z

Hi @powercode, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

iSazonov · 2016-11-19T15:49:05Z

src/System.Management.Automation/engine/Attributes.cs

Why is done a discrimination of attributes? :-)
Why not move it to the base class ValidateArgumentsAttribute?

I think the answer there is localization. For the other attributes it is easy to provide a clear message to the user, but in this case it is not possible.

But it is a good point. Maybe there are times when you want other validation messages than the hardcoded ones.

The localization is not problem because of ErrorMessage is custom message and if it is absent an user will receive the default localized message.

I can see wanting a custom message with a few others. ValidateScript if you don't want to throw, and ValidateSet if you have too many possibilities and a shorter message is clearer.

Added ErrorMessage to ValidateScript and ValidateSet too as suggested by @lzybkr

If you can specify ErrorMessage and it might be ignored, that's poor UX because then you must teach people where it is honored.

I see a bunch of custom validation attributes in this query: https://github.com/search?q=ValidateArgumentsAttribute&type=Code&utf8=%E2%9C%93

Based on the number of interesting hits, I think it would be a mistake to add ErrorMessage to ValidateArgumentsAttribute.

Let's stake out beginning position. Whether bad UX is that here we add ErrorMessage to only one attribute?
If Yes, then we can search for how to do it correctly for all validate attributes in the repo.

If you mean all public validation attributes, then maybe, but I'm not fully convinced.

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.

If you mean all public validation attributes, then maybe, but I'm not fully convinced.

Yes.

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.

Yes, but it is a problem of any public interface that can be used not the way we want. The only thing we can do is make the interface simple and intuitive.

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.

I think this is a minor problem, while we do not generate millions of attributes and not delete them dynamically.

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

Our standard messages are messages of a programmer, they is the exact pronunciation of what attribute checks. They do not contain user-defined semantics. The creator of cmdlet can give its own semantics absolutely any attribute.
Ex.: ValidateNotNullOrEmpty - Please specify how much virtual memory you need for virtual machine.
ValidateDrive - Please specify the drive where connected Game Key.

we need localization support similar to HelpMessageBaseName and HelpMessageResourceId in ParameterAttribute .

Great! I agree.
The only question is whether we can do it without breaking change.

Half-serious, but can we introduce one more class in the hirarchy, like ValidateArgumentsWithErrorMessageAttribute?

powercode · 2016-12-02T14:25:47Z

@TravisEz13 When the label is Review - Waiting for author, I suspect I'm supposed to do something, but in this case I'm not sure what it is. What am I missing?

TravisEz13 · 2016-12-02T22:23:50Z

@powercode Thanks, Just saying you don't know what you need to do will trigger the bot to change the tag. Someone should take a look at the PR soon.

lzybkr

Sorry for losing track of things and not merging this yet.

I'm fine with the code changes - I think selectively adding ErrorMessage is reasonable.

I'd like to see some tests, and at least a help comment in the setter so that someone knows what the arguments in the format string are.

KevinMarquette · 2017-02-28T15:34:17Z

Can we solve this another way? What if we added another field in the property attribute or introduced a new attribute just for the validation message?

lzybkr · 2017-02-28T18:06:41Z

If you read the thread closely - 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.

…dateScript This makes it possibe to write for example [ValidatePattern('[A-Z]:', ErrorMessage='The Drive should be specified as a single letter followed by a colon, for example "D:"')] [string] $Drive, The element being validated is also passed, so {0} can be used in the custom error message

powercode · 2017-03-03T15:41:35Z

I had to make a change to Compiler to make it handle ErrorMessage property on the ValidateSetAttribute.
@lzybkr Can you check that the changes makes sense?
It was first when I added the tests that I understood that was needed :) Good call to request tests!

iSazonov · 2017-03-03T16:37:25Z

test/powershell/Language/Scripting/ParameterBinding.Tests.ps1

All tests is similar so we can use "-TestCases"

How do I do that to change an attribute?

I attach a file with sample.
TestSample.txt

iSazonov · 2017-03-03T16:38:06Z

test/powershell/Language/Scripting/ParameterBinding.Tests.ps1

$ErrorActionPreference ?

Good catch. Leftovers from earlier iteration. Removing.

iSazonov · 2017-03-03T16:44:18Z

test/powershell/Language/Scripting/ParameterBinding.Tests.ps1

'A;B;C' -> 'A,B,C'

iSazonov · 2017-03-03T20:03:57Z

test/powershell/Language/Scripting/ParameterBinding.Tests.ps1

powercode · 2017-04-19T07:16:40Z

@lzybkr comments addressed.

msftclas added the cla-already-signed label Nov 19, 2016

PowerShellTeam added the Review - Needed label Nov 19, 2016

iSazonov reviewed Nov 19, 2016

View reviewed changes

powercode force-pushed the validate-attribute-errormessage branch 3 times, most recently from dcd0183 to bbb9218 Compare November 21, 2016 15:12

HemantMahawar removed the Review-Needed label Dec 1, 2016

TravisEz13 added the Review - Waiting on Author label Dec 1, 2016

PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Dec 2, 2016

lzybkr self-assigned this Feb 23, 2017

lzybkr suggested changes Feb 27, 2017

View reviewed changes

powercode force-pushed the validate-attribute-errormessage branch 2 times, most recently from 0a0cc01 to c0fe998 Compare March 3, 2017 15:35

powercode force-pushed the validate-attribute-errormessage branch from c0fe998 to a38d380 Compare March 3, 2017 15:48

iSazonov reviewed Mar 3, 2017

View reviewed changes

powercode force-pushed the validate-attribute-errormessage branch 2 times, most recently from 3215abd to af1800a Compare March 3, 2017 19:56

iSazonov reviewed Mar 3, 2017

View reviewed changes

test/powershell/Language/Scripting/ParameterBinding.Tests.ps1 Outdated

Copy link

Collaborator

iSazonov Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent.

Adding more descriptive comments for ErrorMessage.

e48dc90

powercode force-pushed the validate-attribute-errormessage branch from af1800a to e48dc90 Compare March 3, 2017 20:08

lzybkr approved these changes Apr 20, 2017

View reviewed changes

lzybkr merged commit fa901b6 into PowerShell:master Apr 20, 2017

powercode mentioned this pull request Apr 23, 2017

Suggestion: Implement optional custom error messages for validation parameter attributes #3630

Closed

powercode deleted the validate-attribute-errormessage branch April 25, 2017 19:00

mklement0 mentioned this pull request May 9, 2017

Implement optional custom error messages for *all* validation parameter attributes (not just ValidateScript and ValidatePattern) #3745

Closed

iSazonov mentioned this pull request May 12, 2017

Support localization of custom error messages for parameter validation attributes #3765

Closed

Adding errormessage property to ValidatePatternAttribute #2728

Adding errormessage property to ValidatePatternAttribute #2728

Uh oh!

Conversation

powercode commented Nov 19, 2016

Uh oh!

msftclas commented Nov 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powercode commented Dec 2, 2016

Uh oh!

TravisEz13 commented Dec 2, 2016

Uh oh!

lzybkr left a comment

Choose a reason for hiding this comment

Uh oh!

KevinMarquette commented Feb 28, 2017

Uh oh!

lzybkr commented Feb 28, 2017

Uh oh!

powercode commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powercode commented Apr 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants