Added setting to execute files in terminal using the file's directory… #316

gandhis1 · 2016-09-17T16:13:51Z

… as the working directory.

Couple of caveats:

I don't use Git or decentralized repos so this is new for me. I also don't code JavaScript or TypeScript but I'm picking this up for fun. Take it easy - I'm new and don't know that much.
Want to go through the process on a trivial, minor setting change. To be fair, I'm actually not a big fan of this change, but I implemented it nonetheless.
I'll be moving into more meaningful bugs/enhancements in the future (for instance, the indentation issue which I previously commented on).

This change addresses #314 , by giving the user a setting of "python.terminal.executeInFileDir", defaulting to False, that, when set to True, will navigate to the directory of the file first, before executing the file in the terminal.

… as the working directory

DonJayamanne · 2016-09-19T23:08:43Z

src/client/providers/execInTerminalProvider.ts

@@ -38,8 +40,13 @@ function execInTerminal(fileUri?: vscode.Uri) {
        filePath = `"${filePath}"`;
    }
    const terminal = vscode.window.createTerminal(`Python`);
+    if (pythonSettings.terminal.executeInFileDir) {


@gandhis1 , this line could fall over if such a setting doesn't exist.
You might want to check if it eixsts as follows (or similar way):

if (pythonSettings.termal && pythonSettings.terminal.executeInFileDir)

I made this change (pushing shortly), but I'm unclear on how exactly that would be possible. The presence of a default value for this setting does not preclude it from not existing?

…into executeInTerminal

…when running

gandhis1 · 2016-09-20T01:14:56Z

Added another setting to support #321. Also, I realized that the terminal does not show and focus when it is run, which leads to awkward situations where a user might not realize they already ran the "Run File" command, when it is hidden, or hidden behind another terminal instance. So I made the terminal appear following the command.

Questions:

Should the "executeInFileDir" setting apply to selections as well?
Is executeInFileDir a robust setting? Seems like it encourages a poor file/folder structure in Python projects, one that is dependent on the absolute, not relative, execution context.

DonJayamanne · 2016-09-20T01:20:49Z

src/client/providers/execInTerminalProvider.ts

+    }
+    const launchArgs = settings.PythonSettings.getInstance().terminal.launchArgs;
+    const launchArgsString = launchArgs.length > 0 ? " ".concat(launchArgs.join(" ")) : "";
+    terminal.sendText(`${currentPythonPath} ${filePath}${launchArgsString}`);


Based on #321, shouldn't the order of the command be as follows?
As we're trying to pass arguments into the python interpreter and not the program.

terminal.sendText(`${currentPythonPath}${launchArgsString} ${filePath}`);

Yes, thanks for spotting my crappy code. I actually did test the code, and incorrectly figured that it was normal behavior for the custom arguments to print nothing to stdout. :-/

DonJayamanne · 2016-09-20T03:52:33Z

@gandhis1

Should the "executeInFileDir" setting apply to selections as well?

Yes

Is executeInFileDir a robust setting? Seems like it encourages a poor file/folder structure in Python projects, one that is dependent on the absolute, not relative, execution context.

Whether you provide the absolute or relative path, its still the same, isn't it? So using absolute paths doesn't matter.
Also, I don't understand how this encourages poor file/folder structures? Please elaborate.

gandhis1 · 2016-09-20T12:34:49Z

What is a valid hypothetical use case for the setting? If you run a Python script normally, it will still be able to access relative paths if you use relative imports and an init.py, which correct me if I'm wrong, but operate off of the file path, rather than the current working directory.

Why does the working directory matter? I can't see a robust use case that wouldn't be better addressed by setting PYTHONPATH instead.

DonJayamanne · 2016-09-20T12:57:16Z

@gandhis1 , I'm confused. Are these new questions related to the execution of selected code or applies to executeInFileDir as well?

which correct me if I'm wrong, but operate off of the file path, rather than the current working directory.

Doesn't the working directory matter if you have code that for some reason access the current working directory. E.g. code using os.getcwd()

Why does the working directory matter? I can't see a robust use case that wouldn't be better addressed by setting PYTHONPATH instead.

See previous answer

gandhis1 · 2016-09-20T13:14:33Z

I'm talking about either run type. Yes, if you use os.getcwd(), it matters. But if you're using os.getcwd() to access resources under the assumption that your working directory = file directory, you have now precluded your script from working under any other execution context. What if you later you turn your script into a module, which is imported from somewhere else, with no context guarantees?

I'm throwing it out there that use cases for this might be better served by using __file__ instead, which is guaranteed regardless of execution context. You see what I'm getting at in terms of this setting potentially encouraging bad practice? Are these concerns unfounded?

DonJayamanne · 2016-09-20T13:33:41Z

@gandhis1 , these are valid questions. However didn't you already start this PR based on some assumptions? What were they? Why start the PR with the ability to set the current working directory?
From what you're saying, we don't need to set the path at all.

Also, isn't #314 a valid user case for this requirement?

gandhis1 · 2016-09-20T14:07:20Z

I wanted to continue the discussion on this and get some feedback from other experienced Python users on the pros/cons of this change. I'm totally OK with this PR getting killed (the executeInFileDir part, not the LaunchArgs part)...just want to hear some reasoning by others, who might have different work contexts (I'm a finance quant).

#314 explains what, but not why. A lot of people are going to request features that may not make sense or lacks an understanding of the bigger picture of why a feature exists and what the appropriate usage case for a given feature is. I suspect this might be one...

From my original post:

DonJayamanne · 2016-09-20T21:55:49Z

@gandhis1, agreed a discussion is warranted.
Personally, I have no preference in this matter.

DonJayamanne · 2016-09-20T22:02:08Z

@gandhis1, sorry if I sounded rude. Just thought had some other scenarios that warranted the PR.
As there's at least one user asking for this feature and you have already done the work, I don't see why we can't proceed with these changes.

If you're happy with the changes, I will merge them, let me know when.

Refactor how environment variables are parsed and used in tools, language server and debugger Fixes #316 (PYTHONPATH not used as extra path for auto complete) Fixes #183 (changes to .env file requires restarts of vscode) Fixes #436 (Path variables not inherited when debugging) Fixes #435 (Virtual environment name is same as the default environment file)

Added setting to execute files in terminal using the file's directory…

08f9e34

… as the working directory

DonJayamanne reviewed Sep 19, 2016

View reviewed changes

gandhis1 added 2 commits September 19, 2016 20:29

Merge branch 'master' of https://github.com/DonJayamanne/pythonVSCode …

c000a91

…into executeInTerminal

Added setting for terminal launch arguments, show and focus terminal …

2853f77

…when running

DonJayamanne reviewed Sep 20, 2016

View reviewed changes

gandhis1 added 2 commits September 19, 2016 21:23

Fixed argument order

66f335e

Thanks to tslinter for fixing JS syntax issues

d7ce09f

DonJayamanne merged commit 76e3613 into DonJayamanne:master Sep 22, 2016

gandhis1 deleted the executeInTerminal branch September 23, 2016 03:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added setting to execute files in terminal using the file's directory… #316

Added setting to execute files in terminal using the file's directory… #316

gandhis1 commented Sep 17, 2016

DonJayamanne Sep 19, 2016

gandhis1 Sep 20, 2016

gandhis1 commented Sep 20, 2016

DonJayamanne Sep 20, 2016

gandhis1 Sep 20, 2016 •

edited

Loading

DonJayamanne commented Sep 20, 2016

gandhis1 commented Sep 20, 2016

DonJayamanne commented Sep 20, 2016 •

edited

Loading

gandhis1 commented Sep 20, 2016

DonJayamanne commented Sep 20, 2016 •

edited

Loading

gandhis1 commented Sep 20, 2016

DonJayamanne commented Sep 20, 2016

DonJayamanne commented Sep 20, 2016

Added setting to execute files in terminal using the file's directory… #316

Added setting to execute files in terminal using the file's directory… #316

Conversation

gandhis1 commented Sep 17, 2016

DonJayamanne Sep 19, 2016

Choose a reason for hiding this comment

gandhis1 Sep 20, 2016

Choose a reason for hiding this comment

gandhis1 commented Sep 20, 2016

DonJayamanne Sep 20, 2016

Choose a reason for hiding this comment

gandhis1 Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

DonJayamanne commented Sep 20, 2016

gandhis1 commented Sep 20, 2016

DonJayamanne commented Sep 20, 2016 • edited Loading

gandhis1 commented Sep 20, 2016

DonJayamanne commented Sep 20, 2016 • edited Loading

gandhis1 commented Sep 20, 2016

DonJayamanne commented Sep 20, 2016

DonJayamanne commented Sep 20, 2016

gandhis1 Sep 20, 2016 •

edited

Loading

DonJayamanne commented Sep 20, 2016 •

edited

Loading

DonJayamanne commented Sep 20, 2016 •

edited

Loading