Improve ValidateCount attribute error message by iSazonov · Pull Request #3596 · PowerShell/PowerShell

iSazonov · 2017-04-19T14:15:30Z

Fix #3585

- Improve ValidateCount attribute error message
- Add tests (blocked by Add autoload for TestLanguage.psm1 TestHelpers.psm1 #3456)

BrucePay

There's a typo in the error message. It should say "is not equal" - the "is" is missing.

iSazonov · 2017-04-20T03:00:20Z

Thanks! Typo is fixed.

lzybkr · 2017-04-20T21:04:07Z

src/System.Management.Automation/resources/Metadata.resx

    <value>The number of provided arguments, ({1}), exceeds the maximum number of allowed arguments ({0}). Provide fewer than {0} arguments, and then try the command again.</value>
  </data>
+  <data name="ValidateCountEqualLengthFailure" xml:space="preserve">
+    <value>The number of provided arguments, ({1}), is not equal the number of allowed arguments ({0}). Provide exactly {0} arguments, and then try the command again.</value>


This is definitely better, but I'm finding all 3 error messages too wordy and slightly inaccurate. First, maybe we should change arguments to values because there is a single argument with multiple values.

Second, maybe 1 error message is sufficient if we change it to:

The parameter requires at least `{0}` value(s) and no more than `{1}` value(s) - `{2}` value(s) were provided.

daxian-dbw · 2017-04-20T22:28:04Z

I edited the changes to correct a typo and remove duplicate test cases.
@iSazonov please see @lzybkr's comment, which I totally agree. But we need to keep the existing fully qualified error ids, otherwise it would be a breaking change.

The typo was fixed in new commits.

iSazonov · 2017-04-21T08:24:58Z

Thanks! I agree that this new message is more clear.

daxian-dbw · 2017-04-21T16:39:22Z

src/System.Management.Automation/resources/Metadata.resx

+    <value>The parameter requires at least `{0}` value(s) and no more than `{1}` value(s) - `{2}` value(s) were provided.</value>
+  </data>
+  <data name="ValidateCountEqualLengthFailure" xml:space="preserve">
+    <value>The parameter requires at least `{0}` value(s) and no more than `{1}` value(s) - `{2}` value(s) were provided.</value>


You don't need 3 resource strings, one would be enough since they are the same. The resource string name could be ValidateCountFailure.

I was very generous 😄
Fixed.

daxian-dbw · 2017-04-21T16:43:16Z

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


+            if (MinLength == MaxLength && len != MaxLength)
+            {
+                throw new ValidationMetadataException("ValidateCountEqualLengthFailure",


The error id ValidateCountEqualLengthFailure sounds confusing. Maybe ValidateCountNotExactlyEqual?

daxian-dbw · 2017-04-21T19:15:11Z

src/System.Management.Automation/resources/Metadata.resx

-  <data name="ValidateCountMaxLengthFailure" xml:space="preserve">
-    <value>The number of provided arguments, ({1}), exceeds the maximum number of allowed arguments ({0}). Provide fewer than {0} arguments, and then try the command again.</value>
+  <data name="ValidateCountFailure" xml:space="preserve">
+    <value>The parameter requires at least `{0}` value(s) and no more than `{1}` value(s) - `{2}` value(s) were provided.</value>


We don't use backtick ` in error strings. I think you can just remove the backticks here.

daxian-dbw · 2017-04-21T19:15:44Z

test/powershell/engine/ValidateAttributes.Tests.ps1

+               @{ sb = { function Local:foo { param([ValidateCount(-1,2)] [string[]] $bar) }; foo         }; FullyQualifiedErrorId = "ExceptionConstructingAttribute";             InnerErrorId = "" }
+               @{ sb = { function Local:foo { param([ValidateCount(1,-1)] [string[]] $bar) }; foo         }; FullyQualifiedErrorId = "ExceptionConstructingAttribute";             InnerErrorId = "" }
+               @{ sb = { function Local:foo { param([ValidateCount(2, 1)] [string[]] $bar) }; foo         }; FullyQualifiedErrorId = "ValidateRangeMaxLengthSmallerThanMinLength"; InnerErrorId = "" }
+               @{ sb = { function Local:foo { param([ValidateCount(2, 2)] [string[]] $bar) }; foo 1       }; FullyQualifiedErrorId = "ParameterArgumentValidationError,foo";       InnerErrorId = "ValidateCountEqualLengthFailure" }


Error id has changed to ValidateCountNotExactlyEqual

daxian-dbw · 2017-04-22T01:00:30Z

Add tests (blocked by #3456)

@iSazonov I'm merging this PR as it has corresponding tests. If you think more tests are needed for it, feel free to submit a new PR.
Like always, thank you for the great fixes!

mklement0 · 2017-04-22T01:39:18Z

It's great that you fixed this so quickly, but while the single error message is now technically accurate, I fear it doesn't provide the best guidance to the user.

As a user who has just mistakenly provided the wrong number of values x, I would want the error message to address my specific error, for a range a to z (simplified wording to illustrate the logic):

If a equals z: "You specified x, but you must specify exactly a."
Otherwise:
- If x < a: "You specified x, but you must specify at least a (and no more than z)."
- If x > z: "You specified x, but you must specify no more than z (but at least a)."

lzybkr · 2017-04-22T04:54:53Z

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

                    null, Metadata.ValidateCountNotInArray);
            }

+            if (MinLength == MaxLength && len != MaxLength)


Isn't this case unnecessary now?

In fact, with one error message, there should be a single throw statement.

We should to keep the two original fully qualified error ids to avoid a breaking change. But the new one I could to remove:

What is your conclusion about last @mklement0 comment?
If you reject it I shall only remove the if (MinLength == MaxLength && len != MaxLength).
If you accept please give me right template.

The fully qualified error ids on parameter validation errors aren't too interesting, I'm not worried about changing those.

Personal opinion - adding prescriptive guidance may have been the cause for the original bug - having a confusing error message. It's easier to get technically accurate errors - what is wrong - than it is to suggest how to fix the problem, so it can be better to not bother. And in this specific case, I think it's fairly obvious how to fix the error.

Thanks! If @daxian-dbw and @mklement0 do not object I'll create new PR tomorrow to only remove unneeded resource strings.

@lzybkr
I think the fundamental question is: Is it a design goal to provide prescriptive guidance? If it is (I think it should be), then let's fix and improve the currently broken / incomplete guidance - and that would mean fixing the 2 broken messages and adding one for the exact-count case.

There are limits to how helpful a generic validation error message can be (see #3630), but here's an opportunity to be as helpful as possible.

Personally, if I specified an incorrect number of values to a parameter that requires an exact number and I got the following message, I'd feel like I'm being presented with a brain-teaser rather than a helpful error message:
The parameter requires at least 2 value(s) and no more than 2 value(s).

@mklement0 - you bring up two different issues - providing clear messages and providing prescriptive guidance.

In this case, one generic message is a bit weird, but felt like a corner case to me. After a quick search - ValidateCount isn't used that much (lots of duplicate hits from forks in the 426 results), but there are examples where Min==Max, so it's not as much of a corner case as I originally thought.

So I guess have 2 messages seems reasonable, but 3 still feels excessive.

My original thinking was - less and simpler code is ideal, and fewer error messages are also good (e.g. it reduces the translation efforts.)

As for prescriptive guidance, it's definitely not a goal for all error messages. To apply that standard widely, you'd end up with messages like Missing closing '}' in statement block or type definition, add the closing '}' and try again. And that feels unnecessary.

@lzybkr: You're right: "prescriptive" was not the right term (and I agree with not needing to go that far) - "specific to the error made" would have been better.

What is a conclusion for new PR - leave one message and remove unneeded resource strings?

@iSazonov: @lzybkr will have to confirm, but here's my understanding:

For the MinLength != MaxLength case: Use the just-added The parameter requires at least {0} value(s) and no more than {1} value(s) - {2} value(s) were provided. message.

For the MinLength == MaxLength case: add a new message along the lines of The parameter requires exactly {0} value(s) - {2} value(s) were provided.

@iSazonov @mklement0 - confirmed.

msftclas added the cla-already-signed label Apr 19, 2017

Improve ValidateCount attribute error message

d52c747

iSazonov force-pushed the validatecount-message branch from 34c9a1e to d52c747 Compare April 19, 2017 14:34

BrucePay previously requested changes Apr 19, 2017

View reviewed changes

Fix typo

354a024

lzybkr reviewed Apr 20, 2017

View reviewed changes

daxian-dbw added 2 commits April 20, 2017 15:18

Change it to "is not equal to" in the error string

3c781ed

Remove duplicate test cases.

45fa925

Change error messages

7642027

daxian-dbw reviewed Apr 21, 2017

View reviewed changes

Remove unneeded resource strings

116e7a8

daxian-dbw reviewed Apr 21, 2017

View reviewed changes

Fix test

13bf4a2

daxian-dbw reviewed Apr 21, 2017

View reviewed changes

Remove the back ticks in resource string

a75c5cc

daxian-dbw merged commit e8ec400 into PowerShell:master Apr 22, 2017

lzybkr reviewed Apr 22, 2017

View reviewed changes

iSazonov mentioned this pull request Apr 27, 2017

Part 2 - Improve ValidateCount attribute error message #3656

Merged

iSazonov deleted the validatecount-message branch October 25, 2017 11:22

Conversation

iSazonov commented Apr 19, 2017 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrucePay left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Apr 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Apr 21, 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!

daxian-dbw commented Apr 22, 2017

Uh oh!

mklement0 commented Apr 22, 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!

mklement0 Apr 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

iSazonov commented Apr 19, 2017 •

edited by daxian-dbw

Loading

daxian-dbw commented Apr 20, 2017 •

edited

Loading

mklement0 Apr 23, 2017 •

edited

Loading