-
Notifications
You must be signed in to change notification settings - Fork 607
Code-type codelenses now prompt for a runtime #1112
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
Conversation
* Returns a set of runtimes for a specified runtime family or undefined if not found. | ||
* @param family Runtime family to get runtimes for | ||
*/ | ||
function getRuntimesForFamily(family: RuntimeFamily): Set<Runtime> | undefined { |
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.
nit: probably makes sense to refactor the various runtime lists into a map later. Off-topic for this PR.
pseudocode:
export runtimes = {
RuntimeFamily.Node: Set<Runtime>(['nodejs12.x', 'nodejs10.x', 'nodejs8.10']),
RuntimeFamily.Python: Set<Runtime>(['python3.8', 'python3.7', 'python3.6', 'python2.7']),
...,
}
This would eliminate nodeJsRuntimes
, pythonRuntimes
, etc.
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.
Will drop a TODO for this. I'm getting sick and tired of the various wrappers we have around suggested runtimes but I'm afraid that it'll make this PR balloon pretty badly.
* Creates a quick pick for a Runtime with the following parameters (all optional) | ||
* @param params buttons: array of buttons to add to the quick pick; | ||
* currRuntime: Runtime to set a "Selected Previously" mark to; | ||
* runtimeFamily: a RuntimeFamily that will define the list of runtimes to show (default: samLambdaCreatableRuntimes) |
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.
Does docstring format have a better way to document these "keywords style" params?
Maybe this is good enough as a hack:
@param buttons ...
@param currRuntime ...
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 this is the correct way to do it: https://jsdoc.app/tags-param.html#parameters-with-properties
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.
FYI, while that's the technically correct way to do it, it doesn't look like Typescript (and thus, VS Code) has implemented it yet: microsoft/TypeScript#11597
Description
Codelenses in source now prompt the user on which runtime to use for the created debug config instead of assuming the newest.
CONSIDERATION: now that we're creating a quick pick, should we also use a quick pick instead of the modal that pops up when using a template codelens with a legacy configuration?
Motivation and Context
Results of a bug bash
Testing
Tested manually with all three runtime families.
Checklist
npm run newChange
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.