-
Notifications
You must be signed in to change notification settings - Fork 24
Adding Truffle Exec context menu #73
base: develop
Are you sure you want to change the base?
Conversation
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 ok, just mostly questions on how you work with the output of the exec call.
Telemetry.sendEvent("TruffleCommands.execScript.truffleInstallation"); | ||
await required.installTruffle(required.Scope.locally); | ||
} | ||
await outputCommandHelper.executeCommand(getWorkspaceRoot(), "npx", RequiredApps.truffle, "exec", uri.fsPath); |
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.
do you not want to do something with the output of this call?
Does this output to the outputChannel or similar? Debugging any issues will be problematic otherwise. Especially any exceptions (non 0 return codes).
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.
hey @michaeljohnbennett (apols for the delay), yup it does currently output to Truffle for VSCode
channel...does that make sense from your perspective?
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.
@kevinbluer, just a heads up: if we have more than one Truffle Project opened, or even a root directory without any truffle-config.js file, the function getWorkspaceRoot() might not work (e.g.: deploy function). The fix for that is on: fix/compile-from-working-dir
await truffleExtensionAdapter.execScript(Uri.file("./test.js")); | ||
|
||
// Assert | ||
assert.strictEqual(execScriptMock.calledOnce, true, "TruffleCommands.execScript should be called once"); |
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.
similarly should this test mock something lower down and return a true/false on the success/failure of the exec call?
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.
possibly 🙃 do you mean whether the script actually executes (irrespective of its success / failure)? i feel this is more of a QOL of update and whether the script actually succeeds / fails doesn't matter too much as long as the output is accessible (as per the above)...it would be awesome if it auto-open to the output channel though
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.
@kevinbluer, looks awesome! :) Only concern is regarding open more than one project and try to execute the scripts. I've left a comment about this function: getWorkspaceRoot()
@kevinbluer what do you want to do with this one? Shall i look at this next week? |
whats the status of this @kevinbluer ? |
Hey @michaeljohnbennett, totally missed the comments here, sorry about that 🤦 Low priority (as a feature) but no harm in getting it added if we can wrangle the merge conflicts. Perhaps we role over to the |
Pretty simple feature that adds a right-click context menu for running
truffle exec path/to/script.js
as per the screenshot.