Add Python Script task #7242
Add Python Script task #7242
Conversation
… in `TaskParameters`, but should be checked at runtime.
Tasks/PythonScriptV0/pythonscript.ts
Outdated
| @@ -0,0 +1,71 @@ | |||
| import * as fs from 'fs'; | |||
| import * as path from 'path'; | |||
| import * as os from 'os'; | |||
| "label": "Arguments", | ||
| "required": false, | ||
| "defaultValue": "", | ||
| "helpMarkDown": "Arguments passed to the script execution, available through `sys.argv`." |
There was a problem hiding this comment.
The Bash task only lets you pass arguments when you're running a script from a file. I don't see any reason for that restriction (in Python at least), so this task lets you pass arguments to inline scripts as well.
Tasks/PythonScriptV0/pythonscript.ts
Outdated
| } else { // Run inline script | ||
| // Print one-line scripts | ||
| const script = parameters.script!; // Required if `targetType` is 'script' | ||
| if (!(script.includes('\n') || script.toLowerCase().includes('##vso['))) { |
There was a problem hiding this comment.
I carried over the one-line restriction from the Bash task, but I don't see any reason for it -- the length of the script is set in the task.json anyway. So I'm just going to log the whole inline script.
There was a problem hiding this comment.
Actually, the script gets logged when you run with system.debug, so I think I'll just remove this.
Tasks/PythonScriptV0/task.json
Outdated
| { | ||
| "name": "filePath", | ||
| "type": "filePath", | ||
| "label": "Script Path", |
There was a problem hiding this comment.
Sentence-cased as "Script path"
Tasks/PythonScriptV0/task.json
Outdated
| ], | ||
| "inputs": [ | ||
| { | ||
| "name": "targetType", |
There was a problem hiding this comment.
In YAML, targetType might be more intuitive if named scriptSource.
There was a problem hiding this comment.
I agree, but this is consistent with the other script tasks.
Tasks/PythonScriptV0/task.json
Outdated
| "label": "Python interpreter", | ||
| "defaultValue": "", | ||
| "required": false, | ||
| "helpMarkDown": "Absolute path to the Python interpreter to use. If not specified, the task will use the interpreter in PATH.", |
There was a problem hiding this comment.
You could append:
"The Use Python Version task can be used to set the version of Python in PATH."
There was a problem hiding this comment.
How about I add this as a "tip" in the docs page? That will keep the help text here brief.
There was a problem hiding this comment.
I like David's suggestion. Many users don't look at docs.
Tasks/PythonScriptV0/task.json
Outdated
| "label": "Fail on standard error", | ||
| "defaultValue": "false", | ||
| "required": false, | ||
| "helpMarkDown": "If this is true, this task will fail if any text are written to the stderr stream.", |
There was a problem hiding this comment.
"any texts are written to"
Tasks/PythonScriptV0/task.json
Outdated
| } | ||
| }, | ||
| { | ||
| "name": "filePath", |
There was a problem hiding this comment.
Can you rename this input from "filePath" to "scriptPath" so that the designer and YAML views match?
Tasks/PythonScriptV0/task.json
Outdated
| { | ||
| "name": "targetType", | ||
| "type": "radio", | ||
| "label": "Type", |
There was a problem hiding this comment.
Can you make this label "Type" match the "name" field?
Tasks/PythonScriptV0/task.json
Outdated
| "label": "Script", | ||
| "visibleRule": "targetType = inline", | ||
| "required": true, | ||
| "defaultValue": "", |
There was a problem hiding this comment.
You could set the default inline script to the following so that it succeeds faster without modification:
# Write your Python script here.\n\n# Add variables marked secret on the Variables tab to pass secret variables to this script.There was a problem hiding this comment.
Synced offline, won't fix for now. We'll hold off on doing secret variables for this task.
Tasks/PythonScriptV0/task.json
Outdated
| "visibleRule": "targetType = inline", | ||
| "required": true, | ||
| "defaultValue": "", | ||
| "properties": { |
There was a problem hiding this comment.
I don't suppose that by some miracle there might be a 'show whitespace' property on this, is there? (probably not...)
There was a problem hiding this comment.
Not that I know of ...
Tasks/PythonScriptV0/pythonscript.ts
Outdated
| const scriptPath = await (async () => { | ||
| if (parameters.targetType.toLowerCase() === 'filepath') { // Run script file | ||
| // Required if `targetType` is 'filepath': | ||
| const filePath = parameters.filePath!; |
There was a problem hiding this comment.
As I understand it, the trailing bang doesn't actually perform a null check. I think it would be an improvement to have an error message that states 'filePath is required'.
There was a problem hiding this comment.
Right -- it's just a compile-time assertion. I can change it to a runtime check.
Tasks/PythonScriptV0/pythonscript.ts
Outdated
| return filePath; | ||
| } else { // Run inline script | ||
| // Required if `targetType` is 'script': | ||
| const script = parameters.script!; |
There was a problem hiding this comment.
parameters.script! => Same as above.
|
It's looking pretty good. Any tests? |
|
None of the other script-runner tasks have tests -- which admittedly is a bad reason not to add tests here. From manual testing I'm pretty confident with it. Since it's the end of sprint, I'd like to go ahead with this commit and add the tests in a separate change. |
Testing