forked from DonJayamanne/pythonVSCode  
 -   Notifications  You must be signed in to change notification settings 
- Fork 1.3k
🐛 Fixes #700 replace back slashes in fie paths with forward slashes #707
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
   Merged  
  DonJayamanne merged 57 commits into microsoft:master from DonJayamanne:execFileOnUnixShellsOnWin        Feb 5, 2018  
    Merged  
 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 7c5778c  Version 0.7.0 of extension (#1381) 
  DonJayamanne 9d1bf82  Update README.md 
  DonJayamanne ffba179  Update README.md 
  DonJayamanne 905c713  sync fork with upstream 
  DonJayamanne acc2109  fix readme 
  DonJayamanne d470523  Merge branch 'master' of https://github.com/Microsoft/vscode-python 
  DonJayamanne d392e8b  merged upstream 
  DonJayamanne 92f775f  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 32a6e53  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 4b30f2c  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne e396752  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne eff4792  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 4553c28  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 3c6520a  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 966e516  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 63d2d65  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne f6d469e  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 029e055  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne e8c71c0  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 51cf9d2  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 7aadc43  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne f0f5c59  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne b2b9da9  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 30a4091  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne b16d2f9  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne c8db345  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 0df7f16  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 3ccc881  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne bb0709e  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 2c19004  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 8f224ab  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 41b7080  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne dab38dc  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne ae22dd4  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne d2340d2  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 52bb7ae  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne b6b2531  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 8d8d2fc  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne c425a55  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 3963217  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne a696f2a  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne a31e659  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 2663cd5  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 7c85e0b  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne beb82c2  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 01e722a  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne d84da8e  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 78da3e1  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 685b683  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 43364fd  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 8701636  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 5c8addf  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 588c2f9  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 66b1382  Merge branch 'master' of https://github.com/DonJayamanne/pythonVSCode 
  DonJayamanne e6b4b48  Merge remote-tracking branch 'upstream/master' 
  DonJayamanne 83ad842  :bug: Fixes #700 replace back slashes in fie paths with forward slashes 
  DonJayamanne File filter
Filter by extension
Conversations
 Failed to load comments.  
    Loading  
 Jump to
  Jump to file  
  Failed to load files.  
    Loading  
 Diff view
Diff view
There are no files selected for viewing
   This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters   
     | Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -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; | ||
| } | ||
|  | ||
| /** | ||
|  | @@ -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(); | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if it is single-quoted? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, '/'); | ||
| }; | ||
   This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters   
        This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters   
        This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters   
        This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters   
     | 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, '/')}"`); | ||
| }); | ||
| }); | 
   This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters   
       Oops, something went wrong.  
  Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.    
 
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 it need to be
this.toCommandArgument(this.ToString())or we don't care about spaces after thetoString()conversion?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.
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).