Skip to content

Implement Get-Error cmdlet as Experimental Feature #10727

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

Merged
merged 23 commits into from
Oct 15, 2019

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Oct 8, 2019

PR Summary

Based on @PowerShell/powershell-committee discussion, we decided to change the cmdlet to Get-Error. RFC to be updated.

Implement Get-Error cmdlet to render errors/exceptions from $error or pipeline. Rendering is recursive for nested objects for Exceptions, InvocationInfo, and Arrays otherwise it uses ToString(). Members that are empty or null are not shown. Indentation is always 4 spaces for nested objects. There is a whitelist of types for nested objects otherwise the output is unusable since most types in .NET are objects and many members are useful for programming but not for visual viewing.

A new FormatAccentColor is introduced to highlight property names from their values. This can be used later to add accents to tables and list formatting. Removed some commented out unneeded code from ConciseView. StackTraces given how long the strings are always left justified instead of indented (the whitespace at the beginning is part of the string, not trimming it).

img

PR Context

Implement Get-Error from PowerShell/PowerShell-RFC#228

PR Checklist

@SteveL-MSFT SteveL-MSFT added the CL-Experimental Indicates that a PR should be marked as an Experimental Feature in the Change Log label Oct 8, 2019
@SteveL-MSFT
Copy link
Member Author

@PoshChan please retry static

@PoshChan
Copy link
Collaborator

@SteveL-MSFT, successfully started retry of PowerShell-CI-static-analysis

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 10, 2019
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 11, 2019
@SteveL-MSFT SteveL-MSFT changed the title Implement Resolve-ErrorRecord cmdlet as Experimental Feature Implement Get-Error cmdlet as Experimental Feature Oct 14, 2019
@rkeithhill
Copy link
Collaborator

Love the new name, the use of color and the completely left-justified stack traces. Nice! Unfortunately, don't love so much the indentation approach to nested exceptions. It is a bit visually hard to follow and I don't think it scales past about 2 (maybe 3) nested exceptions.

We've provided a command like this in the past in PSCX that shows nested exceptions in subsequent sections which I think is easier to follow visually and scales to any number of nested exceptions:

PSMessageDetails      :
Exception             : System.Management.Automation.RuntimeException: Attempted to divide by zero. --->
                        System.DivideByZeroException: Attempted to divide by zero.
                           --- End of inner exception stack trace ---
                           at System.Management.Automation.IntOps.Divide(Int32 lhs, Int32 rhs)
                           at System.Dynamic.UpdateDelegates.UpdateAndExecute2[T0,T1,TRet](CallSite site, T0 arg0, T1                          arg1)
                           at System.Management.Automation.Interpreter.DynamicInstruction`3.Run(InterpretedFrame                               frame)
                           at
                        System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame                          frame)
TargetObject          :
CategoryInfo          : NotSpecified: (:) [], RuntimeException
FullyQualifiedErrorId : RuntimeException
ErrorDetails          :
InvocationInfo        : System.Management.Automation.InvocationInfo
ScriptStackTrace      : at <ScriptBlock>, <No file>: line 1
PipelineIterationInfo : {}

MyCommand             :
BoundParameters       : {}
UnboundArguments      : {}
ScriptLineNumber      : 1
OffsetInLine          : 1
HistoryId             : -1
ScriptName            :
Line                  : 1/0
PositionMessage       : At line:1 char:1
                        + 1/0
                        + ~~~
PSScriptRoot          :
PSCommandPath         :
InvocationName        :
PipelineLength        : 0
PipelinePosition      : 0
ExpectingInput        : False
CommandOrigin         : Internal
DisplayScriptPosition :

Exception at nesting level 0 ---------------------------------------------------

ErrorRecord                 : Attempted to divide by zero.
WasThrownFromThrowStatement : False
Message                     : Attempted to divide by zero.
Data                        : {System.Management.Automation.Interpreter.InterpretedFrameInfo}
InnerException              : System.DivideByZeroException: Attempted to divide by zero.
TargetSite                  : System.Object Divide(Int32, Int32)
StackTrace                  :    at System.Management.Automation.IntOps.Divide(Int32 lhs, Int32 rhs)
                                 at System.Dynamic.UpdateDelegates.UpdateAndExecute2[T0,T1,TRet](CallSite site, T0                                   arg0, T1 arg1)
                                 at
                              System.Management.Automation.Interpreter.DynamicInstruction`3.Run(InterpretedFrame                                     frame)
                                 at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(Inter                               pretedFrame frame)
HelpLink                    :
Source                      : System.Management.Automation
HResult                     : -2146233087

Exception at nesting level 1 ---------------------------------------------------

Message        : Attempted to divide by zero.
Data           : {}
InnerException :
TargetSite     :
StackTrace     :
HelpLink       :
Source         :
HResult        : -2147352558

Now, pair this approach with some coloring to highlight field names and nested exception separators and I think it would work very nicely.

Copy link
Collaborator

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

nothing blocking, just a few questions

@SteveL-MSFT
Copy link
Member Author

@rkeithhill I get your feedback. At this time, I'd like to get this out to users and get additional feedback. We can still provide alternate views.

Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

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

Just one non-blocking question.

@manoj2994
Copy link

So nice that it shows the the way of error and liked the way that null members are not shown,before while tracing errors in the console it is bit pain sometimes by calling members one by one to know the values and the members which haven't .but this shows up all the members only having values thats easy to trace.

Indentation for the nested object is cool ,so that easy to find member to reside on which property.as @rkeithhill suggested about the output for nested object,i personally feel that would be bit confusing while tracking down

@SteveL-MSFT
Copy link
Member Author

@manoj2994 please add your feedback to the RFC PowerShell/PowerShell-RFC#228. I think other visualizations will have their own pros and cons. If an ErrorRecord has multiple nested Exceptions, showing them independently will lose the contextual information that indention provides.

@jpsnover
Copy link
Contributor

jpsnover commented Oct 24, 2019

This is the textbook example for the defined verb "RESOLVE"
That is why all previous community versions of this function are published as Resolve-Error.
We should follow the rules unless there is extra-ordinary circumstances.

@SteveL-MSFT
Copy link
Member Author

@jpsnover the key problem is that AzPS already has a Resolve-Error cmdlet which already caused confusion with users when we originally called it Resolve-ErrorRecord (although ErrorRecord isn't entirely correct as some errors are pure Exceptions).

kilasuit pushed a commit to kilasuit/PowerShell that referenced this pull request Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-Experimental Indicates that a PR should be marked as an Experimental Feature in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants