-
Notifications
You must be signed in to change notification settings - Fork 645
Conversation
@ramya-rao-a Could you have a look at this when you get some time. Thanks! |
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.
Looks like Copy Value
gives the value
being returned here and Copy as Expression
returns the evaluateName
.
Therefore, evaluateName
should be such that the user should be able to the Copy as Expression
, paste it in the Debug Console and get the result.
@ramya-rao-a Fixed it. |
src/debugAdapter/goDebug.ts
Outdated
@@ -816,8 +825,17 @@ class GoDebugSession extends DebugSession { | |||
} | |||
const args = this.delve.isApiV1 ? <DebugVariable[]>outArgs : (<ListFunctionArgsOut>outArgs).Args; | |||
verbose('functionArgs', args); | |||
args.every(local => { |
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 repeated this instead of creating a function because of the const and my understanding is that const variables can't be reassigned right? js noob here.
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.
Because of which const
? The one on args
?
The const only means that you cannot re-assign the array object. But it doesnt stop you from iterating over the contents and changing them.
src/debugAdapter/goDebug.ts
Outdated
@@ -922,6 +943,13 @@ class GoDebugSession extends DebugSession { | |||
variablesReference: 0 | |||
}; | |||
} else { | |||
if(v.children[0].children.length > 0){ |
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.
This is also a repetition since there is a ternary operator doing the same. Is there a cleaner way to do this?
src/debugAdapter/goDebug.ts
Outdated
return { | ||
name: v.name, | ||
value: result, | ||
evaluateName: v.fqn == undefined ? vari.fqn + '.' + v.name : v.fqn, |
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.
Cant we just use v.fqn
here as it already has the right value?
src/debugAdapter/goDebug.ts
Outdated
@@ -157,6 +157,7 @@ interface DebugVariable { | |||
cap: number; | |||
children: DebugVariable[]; | |||
unreadable: string; | |||
fqn: string; |
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 does fqn
stand for?
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.
Fully Qualified Name.Should I change the field name to Fully Qualified Name?
@ramya-rao-a thanks for the review comments. I have incorporated most of your comments. Lemme know if I can do something better? |
@ramya-rao-a could you have a look please :) |
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.
Looks good @karthikraobr!
I pushed a commit with a slight refactoring.
The use of every
on an array is for when you want to check a condition on each item.
If the condition is false
, then the loop exits.
forEach
is when you want to carry out an operation on each item.
Fix for #1990