add .Save() to FileContext API #1139
Conversation
src/features/ExtensionCommands.ts
Outdated
| // Normalize file path case for comparison | ||
| var normalizedFilePath = filePath.toLowerCase(); | ||
|
|
||
| if (vscode.workspace.textDocuments.find(doc => doc.fileName.toLowerCase() == normalizedFilePath)) |
There was a problem hiding this comment.
Careful here. On Linux, the file system is case-sensitive so this test could find the wrong file. Probably need to do a runtime check of the OS or file-system. I wonder if there is some function in node's OS or Path modules that could help?
There was a problem hiding this comment.
Very good point. I actually grabbed that from:
https://github.com/PowerShell/vscode-powershell/blob/master/src/features/ExtensionCommands.ts#L393
So I'll update that as well. Perhaps we should still normalize on Windows?
There was a problem hiding this comment.
Windows and I think macOS, right? Doesn't it also have a case-insensitive file system?
There was a problem hiding this comment.
Ah. I didn't know macOS was case-insensitive! I'll fix the if-check.
src/features/ExtensionCommands.ts
Outdated
| var normalizedFilePath = this.normalizeFilePath(filePath); | ||
|
|
||
| // since Windows is case-insensitive, we need to normalize it differently | ||
| var canFind = vscode.workspace.textDocuments.find(doc => { |
There was a problem hiding this comment.
Probably also good to make this its own function to avoid duplication
There was a problem hiding this comment.
Yep. Looks much cleaner now. Thanks!
|
@rkeithhill when you sign off, I'll get this merged in 😄 |
src/features/ExtensionCommands.ts
Outdated
|
|
||
| private findTextDocument(filePath: string): boolean { | ||
| // since Windows and macOS are case-insensitive, we need to normalize them differently | ||
| var canFind = vscode.workspace.textDocuments.find(doc => { |
There was a problem hiding this comment.
tslint will complain if doc isn't in parens (doc). I've started work with tslint but man, there is soo much change. And somewhere along the line my lint warning fixes broke various commands. Anyway, we should still probably try to avoid adding new stuff for the linter to complain about. :-)
src/features/ExtensionCommands.ts
Outdated
|
|
||
| if (vscode.workspace.textDocuments.find(doc => doc.fileName.toLowerCase() == normalizedFilePath)) | ||
| var promise: Thenable<EditorOperationResponse>; |
There was a problem hiding this comment.
Another linter thing and something @daviwil espoused as well - favor the use of let over var. let is properly blocked scoped in JavaScript. I'm commenting on this one line about it but there are quite a few var usages in this PR that should be let.
There was a problem hiding this comment.
Fixed all occurrences in the file :)
src/features/ExtensionCommands.ts
Outdated
| } else { | ||
| docPath = doc.fileName; | ||
| } | ||
| return docPath == filePath; |
There was a problem hiding this comment.
Probably want to use a === here to prevent any type coercion although both should be strings. I tend to favor === and !== unless I know I want the comparison to do some type coercion for me. See https://stackoverflow.com/questions/359494/which-equals-operator-vs-should-be-used-in-javascript-comparisons
|
Thanks @rkeithhill and @daviwil! |
This is the VSCode portion of this change. See the PSES portion here: PowerShell/PowerShellEditorServices#590
Please let me know if I'm missing something.
To test this, you can use:
$psEditor.GetEditorContext().CurrentFile.Save()in the Integrated Console.