Add better error message for empty and null -UFormat arg#5055
Add better error message for empty and null -UFormat arg#5055iSazonov merged 1 commit intoPowerShell:masterfrom djweber:issue/5047
Conversation
SteveL-MSFT
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Just a few minor comments
There was a problem hiding this comment.
Should have sentence end with period.
There was a problem hiding this comment.
Perhaps have two -TestCases where one is an empty string and the other is just whitespace
Thanks for the feedback! I made the changes you requested. |
There was a problem hiding this comment.
What about null? And I wonder why not use standard [ValidateNotNullOrEmpty()]?
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
" " - is allowed format string.
Disabling Null is breaking change - @SteveL-MSFT What is your thoughts?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@SteveL-MSFT @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
$nullis 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
ValidateNotNullOrEmptyas @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
There was a problem hiding this comment.
I believe ValidateNotNullOrEmpty would best solution if @SteveL-MSFT'd agreed. This breaking change seems incredible.
There was a problem hiding this comment.
Updated PR with proposed changes.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
|
CI MacOs temporary failed - I restart it. Update: failed again on Ruby. |
|
@PowerShell/powershell-committee reviewed this and agree that using the ValidateNotNullOrEmpty attribute is the right fix and having it error with $null is the right behavior although breaking it seems low risk of impact. |
|
@DDWR Please correct your commit - it contains alien code. |
|
@iSazonov Fixed -- had accidentally squashed upstream changes into my old commit. |
There was a problem hiding this comment.
Please add test for $null.
Use TestCases.
There was a problem hiding this comment.
I would suggest putting single quotes around it to make it more readable in the test output:
It "Passing '<name>' to ...
SteveL-MSFT
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Just a few minor comments.
There was a problem hiding this comment.
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 = "" }There was a problem hiding this comment.
@SteveL-MSFT Done. Thank you for the feedback -- it will be helpful moving forward.
There was a problem hiding this comment.
Suggesting removing an and adding whitespace after the empty string (see above)
Previously, when passing an empty format string to -UFormat, a somewhat unhelpful error message would appear. This commit adds a more descriptive error to better guide the user
|
@DDWR Please don't rebase your commits and keep a history. Sometimes we have to rebase to resolve merge conflitcs. Ask maintainers If you are not sure. Welcome with new contributions! - We have tons Issues with and without Up-For-Grabs. |
Previously, when passing an empty format string to -UFormat, a somewhat unhelpful
error message would appear. This commit fixes #5047 by adding a more descriptive error to better
guide the user