Added Addtional Validation Attributes by dchristian3188 · Pull Request #4084 · PowerShell/PowerShell

dchristian3188 · 2017-06-22T22:40:54Z

First pass of additional parameter validations attributes as mentioned here: #2634

New attributes include:
ValidatePositiveAttribute
ValidateNonNegativeAttribute
ValidateNegativeAttribute
ValidateNonPositiveAttribute

lzybkr

We have existing cmdlet parameters where these new attributes should be used.

I think it make sense to start using the new attribute right away.

We also need to think through usage on all data types, and avoid introducing unnecessary new public apis (the base classes).

lzybkr · 2017-06-23T02:06:09Z

test/powershell/engine/ValidateAttributes.Tests.ps1

+    Context "ValidatePositive" {
+        BeforeAll {
+           $testCases = @(
+               @{ sb = { function Local:foo { param([ValidatePositive()] [int] $bar) }; foo -1  }; FullyQualifiedErrorId = "ParameterArgumentValidationError,foo";  InnerErrorId = "ValidateRangeTooSmall" }


local: is unnecessary.

Also - this is hard to read with all keys on the same line - I suggest each key on it's own line.

I'll get this cleaned up. I was trying to follow the existing format of the file. Is it cool to update those other test as part of this PR as well?

lzybkr · 2017-06-23T02:09:53Z

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

+    {
+        /// <summary>
+        /// Minimum allowed value for the attribute
+        /// </summary>


What about other data types? Smaller should just work, but what about long, float, double or bigint?

It's possible dynamic helps you here

In validate element, you would write:

dymamic val = element; if (val > minValue) ...

Any tips on how to include the larger numbers? I was thinking if checking if the element has a CompareTo() but not sure if this is correct. For example, string also have compare to which would be an invalid type.

Try my suggestion to use dynamic - it might just work. Behind the scenes, the C# compiler will generate code calling the correct operator after testing the type of val.

sorry, never used this before. Updated to use dynamic

lzybkr · 2017-06-23T02:17:29Z

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

+    /// <seealso cref="ValidatePositiveAttribute"/>,
+    /// <seealso cref="ValidateNonNegativeAttribute"/>
+    /// </remarks>
+    [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)]


This base class is not generally useful, so I'd prefer avoiding introducing a new public type.

Will remove

iSazonov · 2017-06-23T03:46:31Z

@dchristian3188 Please put a message in #2634 that you are starting the fix to exclude conflicts.

iSazonov · 2017-06-23T04:51:12Z

We also need to think through usage on all data types, and avoid introducing unnecessary new public apis (the base classes).

We could exclude the new apis at all - if we want usage on all data types we should generalize existing ValidateElement(). So we can generalize ValidateRangeAttribute.ValidateElement() and add new constructor to support:

ValidateRange(Range="Positive")

lzybkr · 2017-06-23T17:25:07Z

@iSazonov - I was saying we don't need any unnecessary apis like the abstract base class used to share a tiny bit of code - that could easily be done without inheritance.

Your suggestion might look like:

public enum RangeKind {
    Positive, NonNegative, Negative, NonPositive
}

    // In ValidateRangeAttribute
    RangeKind? _rangeKind;
    public ValidateRange(RangeKind kind) { _rangeKind = kind; }

        // in ValidateElement
        if (_rangeKind.HasValue) { ... }
        else { /* existing code */ }

That might be fine too, I'm not sure I have a preference either way.

iSazonov · 2017-06-23T18:35:04Z

@lzybkr Thanks for you prototype. I like it. We use the similar approach for enhancing ValidateSet with valid values generator. First locally I used inheritance, but your idea (to add new constructor) was better. So I believe we should use the same pattern here. Users will find it easier to know and apply the enhancements than the new apis.

dchristian3188 · 2017-06-23T23:21:36Z

I think you guys are right. Extending Validate range does feel cleaner.

iSazonov

Leave a comment

iSazonov · 2017-06-24T19:03:19Z

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


+    /// <Summary>
+    /// predefined range kind to use with 
+    /// ValidateRangeAttribute


Please start with capital and end with point.
Also we can fit all on one line.

iSazonov · 2017-06-24T19:04:42Z

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

+    /// predefined range kind to use with 
+    /// ValidateRangeAttribute
+    /// </Summary>
+    public enum ValidateRangeKind{


Please add space before bracket.

iSazonov · 2017-06-24T19:06:26Z

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

+    public enum ValidateRangeKind{
+
+        /// <Summary>
+        /// Range is greater than 0


Please end with a point in all comments.

iSazonov · 2017-06-24T19:17:53Z

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

+        /// <Summary>
+        /// Range is less than or equal to 0
+        /// </Summary>        
+        NonPositive


I want to discuss the names. We could use names like math operations:

Name1 Name2 Name3

Positive ValueGT0 GT0

Nonnegative ValueGEzero GEzero

Negative ElementLT0 LT0

NonPositive ElementLEzero LEzero

My Vote is for

1 - Positive, Negative, NonPostive, NonNegative
2 - ValueGTZero, ValueGEZero, ValueGTZero, ValueGEZero

iSazonov · 2017-06-24T19:26:53Z

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

+            if(_rangeKind.HasValue)
            {
-                element = o.BaseObject;
+                dynamic val = element;


Maybe add a comment here what types we expect/process here?

iSazonov · 2017-06-24T19:42:00Z

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

+                switch (_rangeKind)
+                {
+                    case ValidateRangeKind.Positive:
+                        if(val <= 0)


We should use CompareTo() as in original code.

iSazonov · 2017-06-24T19:57:08Z

test/powershell/engine/ValidateAttributes.Tests.ps1

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


Indentation.
And below too.

iSazonov · 2017-06-24T20:02:17Z

test/powershell/engine/ValidateAttributes.Tests.ps1

+        BeforeAll {
+           $testCases = @(
+               @{ 
+                   sb = { function foo { param([ValidateRange('Positive')] [int] $bar) }; foo -1 }


Please add test with the enum:

ValidateRange([ValidateRangeKind]::Positive)

I guess that's enough replace 'Positive' in one test and don't duplicate all tests.

I tried this but it failed since ValidateRangeKind is not exposed as a type accelerator. Should i use the full namespace or add [ValidateRangeKind]?

iSazonov · 2017-06-24T20:05:19Z

test/powershell/engine/ValidateAttributes.Tests.ps1

+           $validTestCases = @(
+               @{ 
+                   sb = { function foo { param([ValidateRange('Positive')] [int] $bar) }; foo 15 }
+                   TestValue = 15


We use LanguagePrimitives.TryConvertTo so we very "generic" here and we should add tests for non-numeric values.

Please add the tests.

iSazonov · 2017-06-24T20:10:14Z

test/powershell/engine/ValidateAttributes.Tests.ps1

+        }
+    }
+
+    Context "ValidateRange - NonNegative" {


I think we can combine all ValidateRangeKind-s in one TestCases:

sb = { function foo { param([ValidateRange( '<TestCase-Parameter-Here>' )] [int] $bar) }; foo -1 }

iSazonov · 2017-06-26T18:32:01Z

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

            {
-                element = o.BaseObject;
+                // Using dynamic to handle different type of numbers such as
+                // int, double, decimal and float. 


CompareTo() allow us be more general. I think we should enhance the comment.

iSazonov · 2017-06-26T18:32:47Z

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

-                else
+
+                // minRange and maxRange have the same type, so we just need
+                // to compare to one of them


Please correct the comment formatting.

iSazonov · 2017-06-26T18:33:42Z

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

+                        element.ToString(), MaxRange.ToString());
+                }
            }
+


Please remove the extra line.

lzybkr

I see you're reusing existing error messages - one reason for this feature was to improve the error message.

And once we've settled on naming, I'd really like to see this new form of the attribute used in our own code base - there are 80 or so places that could use this.

lzybkr · 2017-06-26T22:47:23Z

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

+    /// <Summary>
+    /// Predefined range kind to use with ValidateRangeAttribute.
+    /// </Summary>
+    public enum ValidateRangeKind {


Please follow the conventions of the file you are editing, e.g. braces normally go on their own line, and you have no space after if.

lzybkr · 2017-06-26T22:48:04Z

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

+                switch (_rangeKind)
+                {
+                    case ValidateRangeKind.Positive:
+                        if(val.CompareTo(0) <= 0)


If the argument does not implement IComparable, the error message will not be good.

lzybkr · 2017-06-26T22:49:12Z

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

-                element = o.BaseObject;
+                // Using dynamic to handle different type of numbers such as
+                // int, double, decimal and float. 
+                dynamic val = element;


This might be too general - e.g. strings implement IComparable, but strings aren't positive or negative.

I also see you omit the test for PSObject - you'll probably want it for your code as well - and you should also ensure you have tests to cover simple (non-wrapped) arguments and PSObject wrapped arguments.

This might be too general - e.g. strings implement IComparable , but strings aren't positive or negative.

In general, we cannot know if the type is comparable with zero in advance. Should we test this?

How do you feel about reusing the GetCommonType method from the validate range class?
We could use the incoming object and 0. I believe this will also throw the correct message if the user attempts to pass an incompatable object such as string

That seems reasonable.

Can you share an example where the element was a string and you expected an integer?

Here's what i had for ValidateElement.

protected override void ValidateElement(object element) { if (element == null) { throw new ValidationMetadataException( "ArgumentIsEmpty", null, Metadata.ValidateNotNullFailure); } var o = element as PSObject; if (o != null) { element = o.BaseObject; } if (_rangeKind.HasValue) { Type commonType = GetCommonType((0).GetType(),element.GetType()); ...

Then in PowerShell

function foo { param([ValidateRange('Positive')] $bar) } foo -1

At this point, element will be a string when we enter GetCommonType.

What's interesting is that this does work.

function foo { param([ValidateRange('Positive')] $bar) } foo (-1)

I checked the existing code and they solve this by first getting the common type of the min and max values. They then try to cast element to this new type before comparison operations.

Since I don't have a common type, I was thinking of trying the cast to double.

It is interesting. Maybe it is by design for -1 but confusing:

PS > function foo { param($bar) $bar.Gettype() } PS > foo You cannot call a method on a null-valued expression. At line:1 char:29 + function foo { param( $bar) $bar.Gettype() } + ~~~~~~~~~~~~~~ + CategoryInfo : InvalidOperation: (:) [], RuntimeException + FullyQualifiedErrorId : InvokeMethodOnNull PS > foo 0 IsPublic IsSerial Name BaseType -------- -------- ---- -------- True True Int32 System.ValueType PS > foo -1 IsPublic IsSerial Name BaseType -------- -------- ---- -------- True True String System.Object PS > foo 1 IsPublic IsSerial Name BaseType -------- -------- ---- -------- True True Int32 System.ValueType

In general case, a custom class can implement CompareTo() and allow (CustomClass)0. If the class don't implement this we get an exception - it seems this is acceptable here.

The real problem here is the parameter is not typed - so I think this is by design. If the parameter was typed as [int], then it would have worked as expected.

Based on this, I think it's still reasonable to use GetCommonType.

dchristian3188 · 2017-06-30T05:26:10Z

Ok, i added the check for the common type. Also tried to make the error messages a little more relevant.

iSazonov · 2017-06-30T05:29:37Z

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

+                    null, 
+                    Metadata.ValidateRangeElementType,
+                    element.GetType().Name, 
+                    0.GetType().Name);


0.GetType() and 0.GetType().Name is good condidates for static read only constant.

iSazonov · 2017-06-30T05:35:06Z

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

+
+            if (LanguagePrimitives.TryConvertTo(element, commonType, out resultValue))
+            {
+                elementValue = resultValue as IComparable;


What if we get elementValue == null ?

That shouldn't be possible if there are no bugs in GetCommonType. I'd actually use a cast though so that if it did fail, the error would be better - can't convert instead of null reference.

iSazonov · 2017-06-30T05:44:26Z

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

+
+            if (_rangeKind.HasValue)
+            {
+                ValidatePredefinedRange(element);


Comparing is the same for any range kinds (including "user defined"). I believe we should create a common method to compare elements and call it with different arguments for different ranges.

dchristian3188 · 2017-06-30T06:15:39Z

I get what you're saying about the methods. I've consolidated them into one method with overload. Let me know how it looks.

iSazonov

I meant that a range is a range and we can have one method for all range kinds. ~~We need only add 'ExpliciteRange' in enum.~~

iSazonov · 2017-06-30T10:18:48Z

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

+        ValidateRangeKind? _rangeKind;
+
+        private readonly static Type _zeroType = 0.GetType();
+        private readonly static string _zeroTypeName = 0.GetType().Name;


Please use s_zeroType for static constant.
And maybe _zeroTypeName = s_zeroType.Name;

dchristian3188 · 2017-06-30T23:57:25Z

Hey iSazonov, I'm not sure I follow on adding the additional range type. If we were to add the ExpliciteRange in the enum, wouldn't this allow a user to do something like this:

function foo { param([ValidateRange("ExpliciteRange")] [int] $bar) }; foo 0 }

iSazonov · 2017-07-01T18:10:22Z

@dchristian3188 I agree - we should follow HasValue(). That's enough to make a choice in general method.

iSazonov · 2017-07-02T08:37:58Z

We could enhance and unify Ranges. Currently Min and Max values are allowed but user can want to exclude them. Ex., "Positive" exclude zero. So the "Positive" range looks:

ValidateRange(0, int.MaxValue, RangeOption.ExcludeMin)

"Negative":

ValidateRange( -int.MaxValue, 0, RangeOption.ExcludeMax)

Exclude both (allow 0.5):

ValidateRange( 0, 1, RangeOption.ExcludeMin+RangeOption.ExcludeMax)

And we can move switch (rangeKind) to a constructor (with assigning a specific error message) and implement general ValidateElement method.

dchristian3188 · 2017-07-11T01:09:17Z

Hi iSazonov,

I think expanding the range functionality is a cool idea, but might be out of scope for this PR. Do you want me to open a separate issue for tracking?

Are there other updates I should make to the validate positive, negative, etc?

iSazonov · 2017-07-11T04:21:43Z

@dchristian3188 Please clarify do you plan continue in new PR or want cut off?

dchristian3188 · 2017-07-12T02:30:39Z

Hi iSazonov,

I would continue the work you mentioned in the new PR. If we open the issue, would I be able to get it assigned to me?

iSazonov · 2017-07-12T04:17:13Z

@lzybkr Could you please continue the code review?

lzybkr · 2017-07-02T18:17:17Z

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


+        ValidateRangeKind? _rangeKind;
+
+        private readonly static Type s_zeroType = 0.GetType();


A static is not necessary, you should just use typeof(int) instead.

lzybkr · 2017-07-02T18:26:41Z

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

+        ValidateRangeKind? _rangeKind;
+
+        private readonly static Type s_zeroType = 0.GetType();
+        private readonly static string s_zeroTypeName = s_zeroType.Name;


A static is not necessary here either - the string is only used in error cases, so there is little point in always creating it.

lzybkr · 2017-07-02T18:28:59Z

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

+                    s_zeroTypeName);
+            }
+
+            dynamic elementValue = element;


With this approach, you don't need dynamic.

lzybkr · 2017-07-02T18:30:50Z

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

+
+            if (LanguagePrimitives.TryConvertTo(element, commonType, out resultValue))
+            {
+                elementValue = resultValue as IComparable;


That shouldn't be possible if there are no bugs in GetCommonType. I'd actually use a cast though so that if it did fail, the error would be better - can't convert instead of null reference.

ArieHein · 2017-07-25T09:07:02Z

will this make the value zero validated both as NonNegative and NonPositive ?
or did I miss something ?

dchristian3188 · 2017-07-29T17:19:10Z

Yes 0 will fall into both NonNegative and NonPositive.
The breakdown is:

Positive -gt 0
NonNegative -ge 0
Negative -lt 0
NonPostive -le 0

dchristian3188 · 2017-07-29T17:28:23Z

@lzybkr

Latest PR removes static variables. Also reworked to get rid of the dynamic variable.

Let me know if everything looks ok.

iSazonov · 2017-07-29T18:06:58Z

@dchristian3188 For 0.GetType(); request was

you should just use typeof(int) instead.

dchristian3188 · 2017-07-29T20:49:55Z

hi @iSazonov , sorry missed that one.

Corrections have been made. Please review

ArieHein · 2017-07-29T23:36:10Z

Thers also the issue is 0 defined as positive or negative.

I need to test my DSC scripts to make sure if I use these validations I get the desired logic.

iSazonov

Leave a comment

iSazonov · 2017-07-30T07:26:30Z

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

+                {
+                    dynamicZero = (IComparable)resultValue;
+                    throwTypeError = false;
+                }                    


We can add else and exclude throwTypeError.

iSazonov · 2017-07-30T07:30:20Z

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

    <value>The argument cannot be validated because its type "{0}" is not the same type ({1}) as the maximum and minimum limits of the parameter. Make sure the argument is of type {1} and then try the command again.</value>
  </data>
+  <data name="ValidateRangePositiveFailure" xml:space="preserve">
+    <value>The argument "{0}" cannot be validated because its value is not greater than zero.</value>


We can "validate" but cannot "accept". Maybe better change this. Below too.

You're right, this does make more sense. It also follows the same verbiage as existing Validate Range

dchristian3188 · 2017-07-30T19:01:17Z

Updates made in latest PR

lzybkr

Just a few little things now - I think it's just about ready.

lzybkr · 2017-08-01T20:44:31Z

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

+            _rangeKind = kind;
+        }
+
+        private void ValidateRange(object element, ValidateRangeKind? rangeKind)


It's a private method so it doesn't really matter, but rangeKind is not optional, so the parameter should not be nullable.

lzybkr · 2017-08-01T20:47:00Z

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

+
+            if (LanguagePrimitives.TryConvertTo(element, commonType, out resultValue))
+            {
+                element = (IComparable)resultValue;


I think this cast is pointless.
If the cast were to fail - the error message won't be good.
But I don't think the cast will fail, GetCommonType should not succeed if the type is not IComparable, and the cast isn't necessary when assigning to object.

lzybkr · 2017-08-01T21:16:00Z

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

+                    null, 
+                    Metadata.ValidateRangeElementType,
+                    element.GetType().Name, 
+                    typeof(int).Name);


Shouldn't this be typeof(commonType).Name?

lzybkr · 2017-08-01T21:23:44Z

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

    <value>The argument cannot be validated because its type "{0}" is not the same type ({1}) as the maximum and minimum limits of the parameter. Make sure the argument is of type {1} and then try the command again.</value>
  </data>
+  <data name="ValidateRangePositiveFailure" xml:space="preserve">
+    <value>The argument "{0}" cannot be accepted because its value is not greater than zero.</value>


For consistency, validation errors use The argument "{0} cannot be validated - the cannot be accepted phrase is not commonly used in our code base, basically just in this resx file, and maybe we should change those messages.

lzybkr · 2017-08-01T21:25:19Z

test/powershell/engine/ValidateAttributes.Tests.ps1

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


I realize you didn't introduce this, but sb is not a great name - it is often used for StringBuilder as well. $ScriptBlock is probably better.

I won't block the PR if you want to leave it though.

Revert attribute parameter (as opposed to PowerShell parameter validation) error message

Added Addtional Validation Attributes

e9bebef

msftclas added the cla-already-signed label Jun 22, 2017

daxian-dbw requested a review from lzybkr June 23, 2017 00:51

daxian-dbw assigned lzybkr Jun 23, 2017

daxian-dbw requested a review from iSazonov June 23, 2017 00:51

lzybkr suggested changes Jun 23, 2017

View reviewed changes

dchristian3188 mentioned this pull request Jun 23, 2017

Add ValidatePositive and ValidateNonNegative attributes #2634

Closed

Updated Attributes

ca4125e

iSazonov reviewed Jun 24, 2017

View reviewed changes

Updated ValidateRange tests and code

5e20f99

iSazonov reviewed Jun 26, 2017

View reviewed changes

lzybkr suggested changes Jun 26, 2017

View reviewed changes

Adding addtional element checks. Cleaned up error messages.

31570a0

iSazonov reviewed Jun 30, 2017

View reviewed changes

Cleaned up methods

1c657ce

iSazonov reviewed Jun 30, 2017

View reviewed changes

renamed static variables

f7344f3

lzybkr suggested changes Jul 24, 2017

View reviewed changes

Removed Static and Dynamic Variables

6b39162

Changed to Typeof int

f64f59a

iSazonov reviewed Jul 30, 2017

View reviewed changes

Removed Uneeded variable, changes to Metadata

a8bcc97

lzybkr suggested changes Aug 1, 2017

View reviewed changes

dchristian3188 and others added 3 commits August 2, 2017 07:28

Cleaned up messages and tests

3eca44b

Update Metadata.resx

7ff5f79

Revert attribute parameter (as opposed to PowerShell parameter validation) error message

Update Metadata.resx

1249e78

lzybkr approved these changes Aug 2, 2017

View reviewed changes

lzybkr merged commit 1a003a8 into PowerShell:master Aug 2, 2017

iSazonov mentioned this pull request Aug 3, 2017

Enhance ValidateRange attributes #4477

Closed

iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Aug 3, 2017

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Added Addtional Validation Attributes' MicrosoftDocs/PowerShell-Docs#3096

Closed

joeyaiello removed Documentation Needed in this repo Documentat 9C37 ion is needed in this repo labels Oct 15, 2018

indented-automation mentioned this pull request Mar 5, 2019

Add support for ValidateRangeKind to ParameterMetadata.GetProxyAttributeData #9059

Merged

11 tasks

Name1	Name2	Name3
Positive	ValueGT0	GT0
Nonnegative	ValueGEzero	GEzero
Negative	ElementLT0	LT0
NonPositive	ElementLEzero	LEzero


		ValidateRangeKind? _rangeKind;

		private readonly static Type s_zeroType = 0.GetType();

Conversation

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

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