Conversation
EhabY
left a comment
EhabY
left a comment
There was a problem hiding this comment.
Thanks for contributing to this repo, appreciated
Also make sure to run format/lint:fix (I think the pipeline would fail now)
|
@EhabY happy to contribute 😉 (should pass now 🙏) |
EhabY
left a comment
EhabY
left a comment
There was a problem hiding this comment.
Love the tests! I think we'll have to await Asher's response for the UX but the rest are smaller issues!
| if (isWindows()) { | ||
| await fs.writeFile(echoArgsBin, `@node "${scriptPath}" %*\r\n`); | ||
| } else { | ||
| const content = await fs.readFile(scriptPath, "utf8"); | ||
| await fs.writeFile(echoArgsBin, `#!/usr/bin/env node\n${content}`); | ||
| await fs.chmod(echoArgsBin, "755"); | ||
| } |
There was a problem hiding this comment.
Do we need this? Can't we execute the JS file the same way?
There was a problem hiding this comment.
Not if we want to use 'execFile', which takes the binPath. Is it worth it for testing convenience? 🤔
There was a problem hiding this comment.
Fair enough, I think your solution makes sense. We can add the shebang to the JS file so on non-Windows we just copy the file as is but on Windows we prepend but honestly could go either way
yazan-abu-obaideh
force-pushed
the
feat-750
branch
from
ac27aec to
bcd86f7
Compare
yazan-abu-obaideh
force-pushed
the
feat-750
branch
from
bcd86f7 to
b81afc6
Compare
| if (!this.onAuthRequired) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
We already guard on this so I would throw here instead
|
|
||
| if (!result.ok) { | ||
| if (!result.cancelled) { |
There was a problem hiding this comment.
Nit: flatten this out like:
if(result.cancelled) {
return
}
if (result.ok) {
...
} else {
...
}
| if (!result.cancelled) { | ||
| this.logger.error("Speed test failed", result.error); | ||
| vscode.window.showErrorMessage( | ||
| `Speed test failed: ${result.error instanceof Error ? result.error.message : String(result.error)}`, |
There was a problem hiding this comment.
We already have toError from errorUtils.ts
| binPath: string, | ||
| globalFlags: string[], | ||
| workspaceName: string, | ||
| options: { signal?: AbortSignal; duration?: string }, |
There was a problem hiding this comment.
I don't love mixing two different kinds of options, one for args and one for the execution itself 🤔
Actually maybe inline this, it's very similar to the one in commands#openAppStatus. If we want to separate it out for testability then maybe we use a separate file for this? Something like core/cliExec.ts? (we'd move the version to it)
| const baseUrl = this.requireExtensionBaseUrl(); | ||
| const safeHost = toSafeHost(baseUrl); | ||
| const binary = await this.cliManager.fetchBinary(this.extensionClient); |
There was a problem hiding this comment.
This should use the this.remoteWorkspaceClient since users can be connected to a deployment that is different than the one logged in from the sidebar!
| content: result.value, | ||
| language: "json", | ||
| }); | ||
| vscode.window.showTextDocument(doc); |
There was a problem hiding this comment.
Let's await this one for consistency actually
| return /^\d+[smh]$/.test(v.trim()) | ||
| ? null | ||
| : "Enter a duration like 5s, 10s, or 1m"; |
There was a problem hiding this comment.
The issue here is that this will always be incomplete, this basically accepts ANY Go duration string like 1m30s, maybe we just fail for invalid input?
3 participants
Closes #750