Skip to content
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

🐛 Fixes #700 replace back slashes in fie paths with forward slashes #707

Merged
merged 57 commits into from
Feb 5, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
ecc1ca9
Fix Microsoft/vscode#37627 (#1368)
octref Nov 3, 2017
7c5778c
Version 0.7.0 of extension (#1381)
DonJayamanne Nov 9, 2017
9d1bf82
Update README.md
DonJayamanne Nov 9, 2017
ffba179
Update README.md
DonJayamanne Nov 9, 2017
905c713
sync fork with upstream
DonJayamanne Nov 10, 2017
acc2109
fix readme
DonJayamanne Nov 10, 2017
d470523
Merge branch 'master' of https://github.com/Microsoft/vscode-python
DonJayamanne Nov 16, 2017
d392e8b
merged upstream
DonJayamanne Nov 16, 2017
92f775f
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 20, 2017
32a6e53
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 21, 2017
4b30f2c
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 22, 2017
e396752
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 22, 2017
eff4792
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
4553c28
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
3c6520a
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
966e516
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
63d2d65
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
f6d469e
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 28, 2017
029e055
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 30, 2017
e8c71c0
Merge remote-tracking branch 'upstream/master'
DonJayamanne Nov 30, 2017
51cf9d2
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 1, 2017
7aadc43
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 1, 2017
f0f5c59
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 4, 2017
b2b9da9
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 4, 2017
30a4091
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 5, 2017
b16d2f9
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 6, 2017
c8db345
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 7, 2017
0df7f16
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 8, 2017
3ccc881
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 9, 2017
bb0709e
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 11, 2017
2c19004
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 11, 2017
8f224ab
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 11, 2017
41b7080
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 12, 2017
dab38dc
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 12, 2017
ae22dd4
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 12, 2017
d2340d2
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 14, 2017
52bb7ae
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 15, 2017
b6b2531
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 19, 2017
8d8d2fc
Merge remote-tracking branch 'upstream/master'
DonJayamanne Dec 19, 2017
c425a55
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 3, 2018
3963217
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 3, 2018
a696f2a
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 3, 2018
a31e659
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 4, 2018
2663cd5
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 4, 2018
7c85e0b
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 5, 2018
beb82c2
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 9, 2018
01e722a
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 10, 2018
d84da8e
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 17, 2018
78da3e1
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 19, 2018
685b683
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 23, 2018
43364fd
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 23, 2018
8701636
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 24, 2018
5c8addf
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 25, 2018
588c2f9
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 30, 2018
66b1382
Merge branch 'master' of https://github.com/DonJayamanne/pythonVSCode
DonJayamanne Jan 30, 2018
e6b4b48
Merge remote-tracking branch 'upstream/master'
DonJayamanne Jan 31, 2018
83ad842
:bug: Fixes #700 replace back slashes in fie paths with forward slashes
DonJayamanne Feb 5, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/client/common/extensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ interface String {
* E.g. if an argument contains a space, then it will be enclosed within double quotes.
*/
toCommandArgument(): string;
/**
* Appropriately formats a a file path so it can be used as an argument for a command in a shell.
* E.g. if an argument contains a space, then it will be enclosed within double quotes.
*/
fileToCommandArgument(): string;
}

/**
Expand Down Expand Up @@ -47,5 +52,16 @@ String.prototype.toCommandArgument = function (this: string): string {
if (!this) {
return this;
}
return (this.indexOf(' ') > 0 && !this.startsWith('"') && !this.endsWith('"')) ? `"${this}"` : this.toString();
return (this.indexOf(' ') >= 0 && !this.startsWith('"') && !this.endsWith('"')) ? `"${this}"` : this.toString();

Choose a reason for hiding this comment

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

Does it need to be this.toCommandArgument(this.ToString()) or we don't care about spaces after the toString() conversion?

Copy link
Author

Choose a reason for hiding this comment

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

Don't care about spaces. The change was added to ensure unit test passed, in case we have an empty string with spaces " " (I don't see how that would ever be a valid argument, but I'm supporting that just in case there are some tools that do require it).

Choose a reason for hiding this comment

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

What if it is single-quoted?

Copy link
Author

Choose a reason for hiding this comment

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

We don't use single quotes in the code. its only double quotes used. So this should be ok. The check is there to ensure this doesn't fail if we call this twice.

};

/**
* Appropriately formats a a file path so it can be used as an argument for a command in a shell.
* E.g. if an argument contains a space, then it will be enclosed within double quotes.
*/
String.prototype.fileToCommandArgument = function (this: string): string {
if (!this) {
return this;
}
return this.toCommandArgument().replace(/\\/g, '/');
};
2 changes: 1 addition & 1 deletion src/client/common/terminal/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export class TerminalHelper implements ITerminalHelper {
public buildCommandForTerminal(terminalShellType: TerminalShellType, command: string, args: string[]) {
const isPowershell = terminalShellType === TerminalShellType.powershell || terminalShellType === TerminalShellType.powershellCore;
const commandPrefix = isPowershell ? '& ' : '';
return `${commandPrefix}${command.toCommandArgument()} ${args.join(' ')}`.trim();
return `${commandPrefix}${command.fileToCommandArgument()} ${args.join(' ')}`.trim();
}
public async getEnvironmentActivationCommands(terminalShellType: TerminalShellType, resource?: Uri): Promise<string[] | undefined> {
const settings = this.serviceContainer.get<IConfigurationService>(IConfigurationService).getSettings(resource);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export class DjangoShellCodeExecutionProvider extends TerminalCodeExecutionProvi
const workspaceRoot = workspaceUri ? workspaceUri.uri.fsPath : defaultWorkspace;
const managePyPath = workspaceRoot.length === 0 ? 'manage.py' : path.join(workspaceRoot, 'manage.py');

args.push(managePyPath.toCommandArgument());
args.push(managePyPath.fileToCommandArgument());
args.push('shell');
return { command, args };
}
Expand Down
4 changes: 2 additions & 2 deletions src/client/terminals/codeExecution/terminalCodeExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
const command = this.platformService.isWindows ? pythonSettings.pythonPath.replace(/\\/g, '/') : pythonSettings.pythonPath;
const launchArgs = pythonSettings.terminal.launchArgs;

await this.getTerminalService(file).sendCommand(command, launchArgs.concat(file.fsPath.toCommandArgument()));
await this.getTerminalService(file).sendCommand(command, launchArgs.concat(file.fsPath.fileToCommandArgument()));
}

public async execute(code: string, resource?: Uri): Promise<void> {
Expand All @@ -47,7 +47,7 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
}
public async initializeRepl(resource?: Uri) {
if (this.replActive && await this.replActive!) {
this._terminalService!.show();
await this._terminalService!.show();
return;
}
this.replActive = new Promise<boolean>(async resolve => {
Expand Down
42 changes: 42 additions & 0 deletions src/test/common/extensions.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { expect } from 'chai';
import '../../client/common/extensions';

// Defines a Mocha test suite to group tests of similar kind together
suite('String Extensions', () => {
test('Should return empty string for empty arg', () => {
const argTotest = '';
expect(argTotest.toCommandArgument()).to.be.equal('');
});
test('Should quote an empty space', () => {
const argTotest = ' ';
expect(argTotest.toCommandArgument()).to.be.equal('" "');
});
test('Should not quote command arguments without spaces', () => {
const argTotest = 'one.two.three';
expect(argTotest.toCommandArgument()).to.be.equal(argTotest);
});
test('Should quote command arguments with spaces', () => {
const argTotest = 'one two three';
expect(argTotest.toCommandArgument()).to.be.equal(`"${argTotest}"`);
});
test('Should return empty string for empty path', () => {
const fileToTest = '';
expect(fileToTest.fileToCommandArgument()).to.be.equal('');
});
test('Should not quote file argument without spaces', () => {
const fileToTest = 'users/test/one';
expect(fileToTest.fileToCommandArgument()).to.be.equal(fileToTest);
});
test('Should quote file argument with spaces', () => {
const fileToTest = 'one two three';
expect(fileToTest.fileToCommandArgument()).to.be.equal(`"${fileToTest}"`);
});
test('Should replace all back slashes with forward slashes (irrespective of OS)', () => {
const fileToTest = 'c:\\users\\user\\conda\\scripts\\python.exe';
expect(fileToTest.fileToCommandArgument()).to.be.equal(fileToTest.replace(/\\/g, '/'));
});
test('Should replace all back slashes with forward slashes (irrespective of OS) and quoted when file has spaces', () => {
const fileToTest = 'c:\\users\\user namne\\conda path\\scripts\\python.exe';
expect(fileToTest.fileToCommandArgument()).to.be.equal(`"${fileToTest.replace(/\\/g, '/')}"`);
});
});
6 changes: 3 additions & 3 deletions src/test/common/terminals/activation.bash.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { expect } from 'chai';
import * as path from 'path';
import * as TypeMoq from 'typemoq';
import { EnumEx } from '../../../client/common/enumUtils';
import '../../../client/common/extensions';
import { IFileSystem } from '../../../client/common/platform/types';
import { Bash } from '../../../client/common/terminal/environmentActivationProviders/bash';
import { TerminalShellType } from '../../../client/common/terminal/types';
Expand All @@ -13,7 +14,7 @@ import { IServiceContainer } from '../../../client/ioc/types';

// tslint:disable-next-line:max-func-body-length
suite('Terminal Environment Activation (bash)', () => {
['usr/bin/python', 'usr/bin/env with spaces/env more/python'].forEach(pythonPath => {
['usr/bin/python', 'usr/bin/env with spaces/env more/python', 'c:\\users\\windows paths\\conda\\python.exe'].forEach(pythonPath => {
const hasSpaces = pythonPath.indexOf(' ') > 0;
const suiteTitle = hasSpaces ? 'and there are spaces in the script file (pythonpath),' : 'and there are no spaces in the script file (pythonpath),';
suite(suiteTitle, () => {
Expand Down Expand Up @@ -83,8 +84,7 @@ suite('Terminal Environment Activation (bash)', () => {
// Ensure the path is quoted if it contains any spaces.
// Ensure it contains the name of the environment as an argument to the script file.

const quotedScriptFile = pathToScriptFile.indexOf(' ') > 0 ? `"${pathToScriptFile}"` : pathToScriptFile;
expect(command).to.be.deep.equal([`source ${quotedScriptFile}`.trim()], 'Invalid command');
expect(command).to.be.deep.equal([`source ${pathToScriptFile.fileToCommandArgument()}`.trim()], 'Invalid command');
} else {
expect(command).to.be.equal(undefined, 'Command should be undefined');
}
Expand Down
Loading