Skip to content

Conversation

@djweber
Copy link

@djweber djweber commented Oct 7, 2017

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

@msftclas
Copy link

msftclas commented Oct 7, 2017

CLA assistant check
All CLA requirements met.

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a 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

Copy link
Member

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.

Copy link
Member

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

@djweber
Copy link
Author

djweber commented Oct 8, 2017

Thanks for the contribution! Just a few minor comments
Show outdated src/Microsoft.PowerShell.Commands.Utility/resources/GetDateStrings.resx
Show outdated test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Date.Tests.ps1

Thanks for the feedback! I made the changes you requested.

Copy link
Collaborator

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()]?

Copy link
Author

@djweber djweber Oct 8, 2017

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>

Copy link
Collaborator

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?

Copy link
Author

@djweber djweber Oct 8, 2017

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.

Copy link
Member

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.

Copy link
Author

@djweber djweber Oct 8, 2017

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 $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 @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

Copy link
Collaborator

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.

Copy link
Author

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.

Copy link
Collaborator

@iSazonov iSazonov Oct 9, 2017

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.

Copy link
Member

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.

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 9, 2017

CI MacOs temporary failed - I restart it.

Update: failed again on Ruby.

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision Breaking-Change breaking change that may affect users labels Oct 9, 2017
@SteveL-MSFT
Copy link
Member

@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.

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 9, 2017

@DDWR Please correct your commit - it contains alien code.

@djweber
Copy link
Author

djweber commented Oct 9, 2017

@iSazonov Fixed -- had accidentally squashed upstream changes into my old commit.

Copy link
Collaborator

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.

Copy link
Author

@djweber djweber Oct 9, 2017

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.

@iSazonov iSazonov self-assigned this Oct 9, 2017
Copy link
Member

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 ...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a 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.

Copy link
Member

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 = "" }

Copy link
Author

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.

Copy link
Member

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)

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a 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
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 10, 2017

@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.

@iSazonov iSazonov changed the title Add better error message for empty -UFormat arg Add better error message for empty and null -UFormat arg Oct 10, 2017
@iSazonov iSazonov merged commit aea561f into PowerShell:master Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Get-Date -UFormat '' throws an unhelpful exception for an empty string argument

4 participants