-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix issues when running without debugging and debugged code terminates #249
Conversation
DonJayamanne
commented
Nov 16, 2017
•
edited
Loading
edited
- Fixes Not working Ctrl+F5 for a second time #25, Error "Cannot read property 'openConfigFile' of undefined" when starting python program without the debugger #32, Python: 'Start without debugging shortcut’ has a problem. #35, Ctrl+F5 does not start without debugging #235, Running python code without debugging only works once #242 (unable to run without debugging using CTRL+F5)
- Fixes Debug error when finished running code in integrated/external terminal #191, Debug adapter process has terminated unexpectedly #158, Always getting a "Debug adapter process has terminated unexpectedly" error when using "console" option #24, Debug: Run in External Terminal Returns Error Message on Exit #136 (error message displayed when debugged code terminates)
- Fixes Cannot read property "Threads" of null #157 (debugger crashes when python program does not launch)
- Fixes terminal.external.osxExec not respected as debug terminal? #114, Python / envFile in launch.json causes issues #149, runInTerminalRequest does not work on Mac OS #250 (use vscode infrastructure to launch debugger in integrated and external terminals)
- Fixes Discuss the need for the prompt at the end of the execution of a program #239 Remove prompt added to pause terminal upon program completion
Archive of 0.7.0
* 'master' of https://github.com/Microsoft/vscode-python: Fixes #56 list all environments (#219) Fixes #57 Disable activation on debugging (#220) Fixes #26 Do not run linters when linters are disabled (#222)
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.
You probably want to read https://www.python.org/dev/peps/pep-0008/ 😉
@@ -0,0 +1,150 @@ | |||
# Copyright (c) Microsoft Corporation. All rights reserved. | |||
# Licensed under the MIT License. | |||
|
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.
"""Run a block of code or Python file."""
|
||
import sys | ||
import os | ||
from os import path |
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.
import os.path
.
import visualstudio_py_util as _vspu | ||
except: | ||
traceback.print_exc() | ||
print('''Internal error detected. Please copy the above traceback and report at |
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.
Use """
.
LOAD = _vspu.to_bytes('LOAD') | ||
|
||
def launch(): | ||
# Arguments are: |
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.
Should be in a docstring.
# 6. Startup script name. | ||
# 7. Script arguments. | ||
|
||
# change to directory we expected to start from. |
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.
"Change" (didn't capitalize the comments).
return [this.args.program].concat(programArgs); | ||
protected getLauncherFilePath(): string { | ||
const currentFileName = module.filename; | ||
const ptVSToolsPath = path.join(path.dirname(currentFileName), '..', '..', '..', '..', 'pythonFiles', 'PythonTools'); |
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.
Is there no better way to get to the extension's directory than manually walking backwards?
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.
No, the extension directory is exposed to the extension, but the debugger is launched in a separate process and we don't have access to the same information that the extension has.
sys.stderr.write(out) | ||
sys.stderr.flush() | ||
|
||
def is_same_py_file(file1, file2): |
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.
file_1
, file_2
or something as I barely noticed the two different variable names below with the numbers so close to the text.
// tslint:disable-next-line:member-ordering | ||
protected handleProcessOutput(proc: ChildProcess, failedToLaunch: (error: Error | string | Buffer) => void) { | ||
proc.on('error', error => { | ||
// TODO: This condition makes no sense (refactor) |
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.
How bad would it be to fix this now since we all know TODOs never get done. 😉
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.
agreed
src/client/debugger/Main.ts
Outdated
} | ||
// If launching the integrated terminal is not supported, then defer to external terminal | ||
// If launching the integrated terminal is not supported, then defer to external terminal | ||
// that will be displayed by our own code |
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.
Missing a period.
src/client/debugger/Main.ts
Outdated
} | ||
else { | ||
this.pythonProcess.SendResumeThread(pyThread.Id); | ||
if (this.launchArgs.noDebug !== true) { |
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.
Wouldn't this be !this.launchArgs.noDebug
, or does this have to do with null
/undefined
? Same with the explicit comparisons against boolean values below.
@brettcannon all fixed. |
@brettcannon , I came across a bug when running on Mac OS (I've fixed the issue with 09b3359 and 9616eef, its the last two commit in this PR) and also filed an issue in the vscode repo. |
@brettcannon all comments have been resolved. |
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.
Minor refactoring to do, but I trust you to do it without me blocking you on another round of review.
4. Debug options (not used). | ||
5. '-m' or '-c' to override the default run-as mode. [optional]. | ||
6. Startup script name. | ||
7. Script arguments |
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.
Either add a .
or drop them all from the list.
LOAD = _vspu.to_bytes('LOAD') | ||
|
||
def launch(): | ||
"""Arguments are: |
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.
Add a one-sentence explanation of what this function does.
OUTP = _vspu.to_bytes('OUTP') | ||
LOAD = _vspu.to_bytes('LOAD') | ||
|
||
def launch(): |
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.
Maybe rename to parse_argv()
that teases out the details and then drop the call to run()
? Then in the __name__ == '__main__'
can call parse_argv()
and then call run()
?
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.
Totally agreed, that's cleaner (SOC)