-
Notifications
You must be signed in to change notification settings - Fork 13
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
Typing the name of a variable / function will now print its value ins… #48
Conversation
…tead of nothing. Fixes fzyzcjy#47.
Hi! Thanks for opening this pull request! 😄 |
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.
Good job! Only a few nits and ready to merge
if(response is InstanceRef && response.valueAsString == null) { | ||
// InstanceRef.valueAsString only works on primitive values like String, int, double ETC, so for anything else we have to ask the VM to get the toString value | ||
response = await vm.vmService.evaluate(workspaceIsolate.isolateId, response.id!, "this.toString()"); | ||
} |
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 putting this inside _handleEvaluateResponse
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 putting this inside _handleEvaluateResponse
I was doing that at first, but then I saw that it would require complicating the function and making it asynchronous. But now I wonder if we can append a .toString() call directly to the end of the autogenerated method to simplify this. That second response we're sending is probably slowing things down a little.
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.
Good idea! Feel free to PR that
That second response we're sending is probably slowing things down a little.
btw do you feel it in real usage or is it a theoretical thinking?
@@ -83,9 +83,12 @@ class Executor { | |||
log.info('Phase: Evaluate'); | |||
final isolateInfo = await workspaceIsolate.isolateInfo; | |||
final targetId = isolateInfo.rootLib!.id!; | |||
final response = await vm.vmService | |||
Response response = await vm.vmService |
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.
... and making it using the original keyword final
again
.evaluate(workspaceIsolate.isolateId, targetId, _evaluateCode); | ||
|
||
if(response is InstanceRef && response.valueAsString == null) { |
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.
And please add a test in e2e_test.dart, which is quite simple - just provide some inputs and assert outputs
@all-contributors please add @Keithcat1 for code |
I've put up a pull request to add @Keithcat1! 🎉 |
Shouldn't every expression output its value and not just variables and functions? Or is that already what this PR will do? |
@Quijx expressions already output values. e.g. Anyway, feel free to PR to add a few tests that you think you want the behavior |
Hi! Congrats on merging your first pull request! 🎉 |
…tead of nothing. Fixes #47.