Add better error message for empty and null -UFormat arg - #5055

Should have sentence end with period.

Perhaps have two -TestCases where one is an empty string and the other is just whitespace

What about null? And I wonder why not use standard [ValidateNotNullOrEmpty()]?

It seems that [ValidateNotNullOrEmpty()] does not check for whitespace (" " or the like), so it looks like that has to be checked manually. If $null is passed to -uformat, it will just fall through an if-else completely and just print the date with a default format:

PS /Users/david> get-date -uformat $null

Sunday, October 8, 2017 7:01:08 AM


PS /Users/david>

" " - is allowed format string.
Disabling Null is breaking change - Steve Lee (@SteveL-MSFT) What is your thoughts?

Thanks for the feedback. Just to clarify, I haven't changed the behavior of $null being passed in. I did not know that " " was a valid format string -- I assumed that this was also considered empty.

Since $null is the same as if -UFormat wasn't provided, we probably don't want to break that behavior as someone may be relying on it. -UFormat enables custom formatting, so any valid string should work. I suppose it's not different if they provided " " or "apple" which should assume they know what they are doing. So it seems that in this case, the only fix is on this line if UFormat.Length == 0, just return.

Steve Lee (@SteveL-MSFT) Ilya (@iSazonov) Thank you for the responses. Based off this discussion, can we clarify the desired behavior? Based off of what has been said, this is the behavior it seems that we want here:

Current

• If $null is passed in, fall through and just print the date
• If ' ', apple, or any unconventional date format of length > 0 is passed in, keep the current behavior and show the formatted result.

Proposed additions

We can go one of these three routes:

• Add a length check to this line, which will cause it to match the above behavior and format the date to an empty string, no longer throwing an error. This changes the current behavior.
• Since whitespace is supported, use ValidateNotNullOrEmpty as Ilya (@iSazonov) suggested, which will throw an error that can be tested. I think this is the best option, since it produces a testable error for the empty format string, and gives a more descriptive error message without changing the current behavior. No need for extra strings, either.
• Use ValidateLength, same as above, although this requires an upper limit to be specified

I believe ValidateNotNullOrEmpty would best solution if Steve Lee (@SteveL-MSFT)'d agreed. This breaking change seems incredible.

Updated PR with proposed changes.

Steve Lee (@SteveL-MSFT) I think the uformat is a static constant in most scenarios. If someone dynamically forms it, it's not likely that it will leave it uninitialized (null). So I believe this is a breaking change only formally and we can accept it.

I agree that most likely if $null gets passed, it's probably a problem in someone's script. Let me get a quick ack from @PowerShell/powershell-committee before we take this breaking change.

Please add test for $null.
Use TestCases.

Ilya (@iSazonov) I made the changes you requested.

I would suggest putting single quotes around it to make it more readable in the test output:

It "Passing '<name>' to ...

Steve Lee (@SteveL-MSFT) Done

Please change "null" to "$null" (use backtick to escape the dollar sign). Also add some whitespace to align value with test case below to make it easier to read:

@{ name = "`$null"      ; value = $null }
@{ name = "empty string"; value = "" }

Steve Lee (@SteveL-MSFT) Done. Thank you for the feedback -- it will be helpful moving forward.

Suggesting removing an and adding whitespace after the empty string (see above)