Skip to content

Conversation

@remcohaszing
Copy link

@remcohaszing remcohaszing commented Aug 26, 2022

parseArgs is used as a command line argument parser, because it polyfills an experimental Node.js API. Eventually this means this dependency can be removed.

Closes #81

`parseArgs` is used as a command line argument parser, because it polyfills an experimental Node.js API. Eventually this means this dependency can be removed. `@types/node` was updated to version 18 in order to provide types for `@pkgjs/parseargs`. Closes microsoft#81
lib/bin.ts Outdated
Options
--vscode-executable-path The VS Code executable path used for testing.
--version The VS Code version to download.
Copy link
Author

Choose a reason for hiding this comment

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

This flag is a bit weird IMO. --version usually prints the version of a tool, but in this case it matches the version option of runTests. Any suggestions?

Choose a reason for hiding this comment

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

I would probably rename it to --vscode-version. PS: I'm not a maintainer.

Copy link
Author

Choose a reason for hiding this comment

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

I agree. I’m glad to see someone mention this, regardless whether you’re a maintainer or not. :)

I changed it to --vscode-version, and repurposed the --version flag to print version information.

@remcohaszing
Copy link
Author

Ping @connor4312! Given your recent contributions I thought maybe you had overseen this PR.

'vscode-version': { type: 'string' },
platform: { type: 'string' },
'reuse-machine-install': { type: 'boolean' },
'extension-development-path': { type: 'string', multiple: true },

Choose a reason for hiding this comment

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

Did you intentionally miss this one in help? Also, is it intentionally made "multiple"?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! I accidentally missed that one.

It’s multiple, because runTests() accepts it as an array.

@connor4312
Copy link
Member

Thanks for the PR! I opted to go with a bit more involved approach in #235

Namely, the CLI 'mandates' a Mocha setup that we'll be able to use to have UI integration for running extension tests. This solves a problem with the current test setup where the runner is entirely user-defined, with unknown ways of handling arguments or filtering tests. This lack of prescription, while good to give power to users (and not an interface we'll ever remove), meant we couldn't really build anything on top of it. I wanted to take the CLI as an opportunity to solve this problem.

Again, thanks for your contribution of code and ideas, and I hope you can provide some feedback on the alternative approach above 🙂

@connor4312 connor4312 closed this Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants