-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Add better error message for empty and null -UFormat arg #5055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
SteveL-MSFT
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Just a few minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have sentence end with period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about null? And I wonder why not use standard [ValidateNotNullOrEmpty()]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" " - is allowed format string.
Disabling Null is breaking change - @SteveL-MSFT What is your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe ValidateNotNullOrEmpty would best solution if @SteveL-MSFT'd agreed. This breaking change seems incredible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated PR with proposed changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test for $null.
Use TestCases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iSazonov I made the changes you requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest putting single quotes around it to make it more readable in the test output:
It "Passing '<name>' to ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SteveL-MSFT Done
SteveL-MSFT
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. Just a few minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SteveL-MSFT Done. Thank you for the feedback -- it will be helpful moving forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggesting removing an and adding whitespace after the empty string (see above)
SteveL-MSFT
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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