Skip to content

Conversation

@rkeithhill
Copy link
Contributor

Invoke-Formatter -ScriptDefinition '' - throws a terminating error:
ParameterBinderValidationException.

I suppose we could have added another catch statement to the InvokePowerShell method
in the AnalysisService but in general, we probably want to know about such bugs.

We could also update the extension to not send a format request for an empty doc
but I'm not sure it's worth the additional code change given this corner case scenario.
But I could be convinced otherwise.

This is a partial fix to vscode issue PowerShell/vscode-powershell#1346 (comment)

Invoke-Formatter -ScriptDefinition '' - throws a terminating error:
ParameterBinderValidationException.

I suppose we could have added another catch statement to the InvokePowerShell method
in the AnalysisService but in general, we probably want to know about such bugs.

Also, we could also update the extension to not send a format request for an empty doc
but I'm not sure it's worth the additional code change given corner case scenario.
But I could be convinced otherwise.
@rkeithhill rkeithhill changed the title Fix PSES crash that happens if you format an empty PS doc Fix PSES crash when format an empty PS doc Jun 11, 2018
@rkeithhill rkeithhill changed the title Fix PSES crash when format an empty PS doc Fix PSES crash when you format an empty PS doc Jun 11, 2018
Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

LGTM. Agreed on not catching the exception -- it's much more performant this way, and doing it the other way would probably just pollute the logs.

Also, I agree with handling it on the server side -- if the behaviour of Invoke-Formatter were to change, it would make more sense to me to make the change here.

@rkeithhill rkeithhill merged commit f035aed into master Jun 13, 2018
@rkeithhill rkeithhill deleted the rkeithhill/fix-formatter-crash-empty-string branch June 13, 2018 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants