Add CurrentFile SaveAs() support #1261

rjmholt · 2018-04-06T21:16:24Z

In combination with PSES #650, this adds the ability to run $psEditor.GetEditorContext().CurrentFile.SaveAs("newFile").

Some things to note (that I'm looking for input on):

The SaveAs method supports both absolute and relative paths, relative being relative to the current file path.
I've been trying to make VSCode open the newly saved file in a a tab/window, but after trying quite a few different things, it seems to really want to close the window. Any ideas?
There are a few ways the saving can fail. My current solution is for PSES to handle that possibility with pre-validation, but maybe we should send a success/fail message back instead?

rjmholt · 2018-04-10T22:52:03Z

Yeah, getting rid of the nesting is tough because you're building context as you go (it's secretly a monad).

But I think you're quite right, we can reduce the complexity by loosening the sequencing requirements.

rjmholt · 2018-04-10T23:12:54Z

I think I tried using saveFileDetails.newPath but it opens a strange editor window which isn't really native. I had to pass it the new document handle to get it to work.

TylerLeonhardt · 2018-04-12T02:53:15Z

src/features/ExtensionCommands.ts

+                                });
+                            });
+                    });
+            });


Needs a nice syntactic sugar I think...

TylerLeonhardt · 2018-04-12T02:56:38Z

I'm considering posting on Stack Overflow on how one might saveAs using the vscode API. I feel like it's a reasonable question and people would have use of it. Thoughts?

rjmholt · 2018-04-12T02:58:37Z

Yeah I agree. Either there or on their GitHub page

TylerLeonhardt · 2018-04-12T03:10:52Z

Here it is: https://stackoverflow.com/questions/49787354/implementing-a-save-as-using-the-vscode-api

We can open an issue in a day or so if we don't get any bites.

SeeminglyScience · 2018-04-12T03:14:54Z

Any reason an async method won't work?

private async saveFile(saveFileDetails: ISaveFileDetails): Promise<EditorOperationResponse> {

    // If the file to save can't be found, just complete the request
    if (!this.findTextDocument(this.normalizeFilePath(saveFileDetails.filePath))) {
        return EditorOperationResponse.Completed;
    }

    // If no newFile is given, just save the current file
    if (!saveFileDetails.newPath) {
        const doc = await vscode.workspace.openTextDocument(saveFileDetails.filePath);
        if (doc.isDirty) {
            await doc.save();
        }
        return EditorOperationResponse.Completed;
    }

    // Otherwise we want to save as a new file

    // First turn the path we were given into an absolute path
    // Relative paths are interpreted as relative to the original file
    const newFileAbsolutePath = path.isAbsolute(saveFileDetails.newPath) ?
        saveFileDetails.newPath :
        path.resolve(path.dirname(saveFileDetails.filePath), saveFileDetails.newPath);

    // Create an "untitled-scheme" path so that VSCode will let us create a new file with a given path
    const newFileUri = vscode.Uri.parse("untitled:" + newFileAbsolutePath);

    // Now we need to copy the content of the current file,
    // then create a new file at the given path, insert the content,
    // save it and open the document
    const oldDoc = await vscode.workspace.openTextDocument(saveFileDetails.filePath);
    const newDoc = await vscode.workspace.openTextDocument(newFileUri);
    const editor = await vscode.window.showTextDocument(newDoc, 1, false);
    await editor.edit((editBuilder) => {
        editBuilder.insert(new vscode.Position(0, 0), oldDoc.getText());
    });
    await newDoc.save();
    return EditorOperationResponse.Completed;
}

Edit: Completely missed the part about it not working in general. Whoops :)

rjmholt · 2018-04-12T03:17:06Z

Wow, that's much better. I had no idea async was a thing in TypeScript...

rjmholt · 2018-04-12T03:21:36Z

Now the question remains, how do we bring the new document window into focus. (The async behaviour could change it, but I imagine it won't). Currently, the new window comes into focus, saves, and is closed again. Leaving us looking at the unsaved old window. But I'm imagining that when I save-as a file, I want to see the new saved document open in front of me... (Unless that seems wrong?)

TylerLeonhardt · 2018-04-12T03:25:52Z

didn't realize we could use async/await here 😄 thanks for that @SeeminglyScience!

TylerLeonhardt · 2018-04-12T03:30:25Z

I wonder if it's because you're saving an untitled file:

const newFileUri = vscode.Uri.parse("untitled:" + newFileAbsolutePath);

rjmholt · 2018-04-12T16:45:08Z

I've been trying various ways to "save-as" and then open that window.

Opening with a new "file://" URI after the save, I get the new file opened. But then the old one closes, which isn't what I imagine people wanting.

I'm mildly reluctant to bypass VSCode and just use fs like the SO answer suggests, but it might be much easier...

rjmholt · 2018-04-16T17:47:57Z

Ok, I've fixed up the code (resolved by pulling the text out of the unsaved buffer, saving it to the filesystem and the opening the new file...) and it now seems to do what I want. We should probably test it out a bit and validate the behaviour (and see if using different paths breaks it) before merging, but I think it's good to go.

rkeithhill · 2018-04-16T20:01:41Z

I wonder if we should just ignore the package-lock.json file altogether? We've started doing this at work as this file kept getting changed by everyone's dev environment. We could nuke this by adding a .npmrc with this in it:

package-lock=false

rjmholt · 2018-04-16T20:48:45Z

Yeah, I had added it to the ignore on the basis that it was generated by the user's environment, but @tylerl0706 suggested we should keep it in as it represents some pre-resolved work for npm.

npm's pages state that it's intended to be tracked, but if it keeps changing in every commit because different machines use it differently, then I'd argue that it's noise in the commit and we should ignore it (and npm should work out what they're trying to achieve with this file...). But I don't have a strong opinion either way (as long as deleting package-lock.json doesn't get us into dependency hell or something).

TylerLeonhardt

async/await is beautiful and this LGTM! Just a couple questions that I'm curious about.

TylerLeonhardt · 2018-04-16T22:55:50Z

src/features/ExtensionCommands.ts

+        const oldDocText = oldDocument.getText();
+
+        // Write it to the new document path
+        fs.writeFileSync(newFileAbsolutePath, oldDocText);


Nitpick:

fs.writeFileSync(newFileAbsolutePath, oldDocument.getText());

deletes a line.

might be worth it but don't care that much.

TylerLeonhardt · 2018-04-16T22:57:01Z

src/features/ExtensionCommands.ts

+        // Finally open the new document
+        const newFileUri = vscode.Uri.file(newFileAbsolutePath);
+        const newFile = await vscode.workspace.openTextDocument(newFileUri);
+        vscode.window.showTextDocument(newFile, { preview: false });


Why did you choose to open it not as a preview? Just curious. Not really leaning either way personally.

Ah, didn't know what it was (just followed the response on SO figuring it prevented some behaviour). I feel like maybe it actually should be a preview...

I'm leaning on preview actually too 😄

TylerLeonhardt · 2018-04-16T22:58:50Z

src/features/ExtensionCommands.ts

@@ -382,24 +388,45 @@ export class ExtensionCommandsFeature implements IFeature {
        return promise;
    }

-    private saveFile(filePath: string): Thenable<EditorOperationResponse> {
+    private async saveFile(saveFileDetails: ISaveFileDetails): Promise<EditorOperationResponse> {


Did you have to switch this from a Thenable to a Promise to use async/await?

Yeah -- as in @SeeminglyScience's snippet. Thenable doesn't type check.

good to know!

TylerLeonhardt · 2018-04-17T01:03:57Z

src/features/ExtensionCommands.ts

+        // Finally open the new document
+        const newFileUri = vscode.Uri.file(newFileAbsolutePath);
+        const newFile = await vscode.workspace.openTextDocument(newFileUri);
+        vscode.window.showTextDocument(newFile, { preview: true });


preview should be true by default. Were you seeing otherwise?

No no, and actually I originally just took it out. But I couldn't find a reference to what the default is in the documentation and decided that we may as well make the expected behaviour explicit.

Sounds good!

brwilkinson · 2018-04-27T17:53:11Z

I have tested out the SaveAs('d:\test.ps1')

This appears to work if the file is already saved as another file, however not if the file is Untitled?

E.g.
Path : untitled:Untitled-1
WorkspacePath : untitled:Untitled-1

Is someone able to test this and see if they have the same experience ?

rjmholt · 2018-04-27T21:16:47Z

You're quite right. I've opened an issue: #1303.

Add file saveas support

c07f5e9

rjmholt mentioned this pull request Apr 6, 2018

Add $psEditor.GetEditorContext().CurrentFile.SaveAs("Name") support PowerShell/PowerShellEditorServices#650

Merged

TylerLeonhardt reviewed Apr 12, 2018

View reviewed changes

Convert saveas to async method

80e73f4

Fix savas by using file system, return package-lock.json

64b936d

TylerLeonhardt approved these changes Apr 16, 2018

View reviewed changes

rjmholt added 2 commits April 16, 2018 16:07

Remove preview false flag

5905ca4

Make preview setting explicit

5f9180c

TylerLeonhardt reviewed Apr 17, 2018

View reviewed changes

TylerLeonhardt merged commit 335acd3 into PowerShell:master Apr 18, 2018

rjmholt mentioned this pull request Apr 18, 2018

Please add extra functionality to $pseditor similar to $psise #773

Closed

rjmholt deleted the filecontext-saveas branch December 11, 2019 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CurrentFile SaveAs() support #1261

Add CurrentFile SaveAs() support #1261

rjmholt commented Apr 6, 2018

rjmholt commented Apr 10, 2018

rjmholt commented Apr 10, 2018

TylerLeonhardt Apr 12, 2018

rjmholt Apr 12, 2018

TylerLeonhardt commented Apr 12, 2018 •

edited

Loading

rjmholt commented Apr 12, 2018

TylerLeonhardt commented Apr 12, 2018 •

edited

Loading

SeeminglyScience commented Apr 12, 2018 •

edited

Loading

rjmholt commented Apr 12, 2018 •

edited

Loading

rjmholt commented Apr 12, 2018

TylerLeonhardt commented Apr 12, 2018

TylerLeonhardt commented Apr 12, 2018

rjmholt commented Apr 12, 2018

rjmholt commented Apr 16, 2018 •

edited

Loading

rkeithhill commented Apr 16, 2018

rjmholt commented Apr 16, 2018

TylerLeonhardt left a comment

TylerLeonhardt Apr 16, 2018

TylerLeonhardt Apr 16, 2018

rjmholt Apr 16, 2018

TylerLeonhardt Apr 16, 2018

TylerLeonhardt Apr 16, 2018

rjmholt Apr 16, 2018

TylerLeonhardt Apr 16, 2018

TylerLeonhardt Apr 17, 2018

rjmholt Apr 17, 2018 •

edited

Loading

TylerLeonhardt Apr 17, 2018

brwilkinson commented Apr 27, 2018

rjmholt commented Apr 27, 2018

Add CurrentFile SaveAs() support #1261

Add CurrentFile SaveAs() support #1261

Conversation

rjmholt commented Apr 6, 2018

rjmholt commented Apr 10, 2018

rjmholt commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt commented Apr 12, 2018 • edited Loading

rjmholt commented Apr 12, 2018

TylerLeonhardt commented Apr 12, 2018 • edited Loading

SeeminglyScience commented Apr 12, 2018 • edited Loading

rjmholt commented Apr 12, 2018 • edited Loading

rjmholt commented Apr 12, 2018

TylerLeonhardt commented Apr 12, 2018

TylerLeonhardt commented Apr 12, 2018

rjmholt commented Apr 12, 2018

rjmholt commented Apr 16, 2018 • edited Loading

rkeithhill commented Apr 16, 2018

rjmholt commented Apr 16, 2018

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brwilkinson commented Apr 27, 2018

rjmholt commented Apr 27, 2018

TylerLeonhardt commented Apr 12, 2018 •

edited

Loading

TylerLeonhardt commented Apr 12, 2018 •

edited

Loading

SeeminglyScience commented Apr 12, 2018 •

edited

Loading

rjmholt commented Apr 12, 2018 •

edited

Loading

rjmholt commented Apr 16, 2018 •

edited

Loading

rjmholt Apr 17, 2018 •

edited

Loading