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

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

Merged
merged 1 commit into from
Apr 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,15 @@ public static readonly
public class SaveFileRequest
{
public static readonly
RequestType<string, EditorCommandResponse, object, object> Type =
RequestType<string, EditorCommandResponse, object, object>.Create("editor/saveFile");
RequestType<SaveFileDetails, EditorCommandResponse, object, object> Type =
RequestType<SaveFileDetails, EditorCommandResponse, object, object>.Create("editor/saveFile");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this be a breaking change? I doubt anyone is using this request directly, but it's worth noting IMessageSender is part of the public API as it's available from $psEditor.Components

Copy link
Member

Choose a reason for hiding this comment

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

Good point. This will be a part of a 1.7.0 release so we'll just have to be explicit and say that there's a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

unless there's a way to be backcompat friendly?

Copy link
Contributor Author

@rjmholt rjmholt Apr 17, 2018

Choose a reason for hiding this comment

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

Good point -- we really should start thinking of the API itself as the contract, not the VSCode behaviour. (Especially since I want to start working on a small vim client).

I'll change it back for the next 1.x release and then we can change it back again for 2.0. Does that sound reasonable?

(We don't really need to change it for 2.0 -- was just going for a consistent minimal API)

Copy link
Member

Choose a reason for hiding this comment

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

breaking changes this small aren't bad. We just need to make sure we let people know in the release notes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah ok -- I think that's fair. Especially after all the people asking for SaveAs().

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah definitely not a big deal. I highly doubt there is a single person using that request directly with IMessageSender right now. Just something we should really keep an eye on as PSES is just starting to see some adoption by other folks.

I could see SaveAs being a separate request if you wanted to be sure we don't mess anyone up, but I'm not sure how much refactoring that'd be. Probably not worth it.

Copy link
Contributor Author

@rjmholt rjmholt Apr 17, 2018

Choose a reason for hiding this comment

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

Yeah, I was thinking about setting it back to use a different request type, but now @tylerl0706 has convinced me that it's more effort than it's worth. But definitely agree that we need to keep the API stable.

I remember looking at the Save() API and thinking whether SaveAs() should be implemented independently and deciding that it makes sense (in a world without downstream developers) as just an additional field.

In general though, one thing I think might be worth doing is changing any API calls that we might want to extend which use a primitive (e.g. string) body to using an object body with a field, just so that we can conceivably add a new field and break the API "less"...

Copy link
Member

Choose a reason for hiding this comment

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

That last paragraph is awesome. Reminds me of graphQL in a lot of ways

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shudder


public class SaveFileDetails
{
public string FilePath { get; set; }

public string NewPath { get; set; }
}

public class ShowInformationMessageRequest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,20 @@ public Task CloseFile(string filePath)
}

public Task SaveFile(string filePath)
{
return SaveFile(filePath, null);
}

public Task SaveFile(string currentPath, string newSavePath)
{
return
this.messageSender.SendRequest(
SaveFileRequest.Type,
filePath,
new SaveFileDetails
{
FilePath = currentPath,
NewPath = newSavePath
},
true);
}

Expand Down
23 changes: 23 additions & 0 deletions src/PowerShellEditorServices/Extensions/FileContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
//

using System;
using System.IO;
using System.Linq;
using System.Management.Automation.Language;

Expand Down Expand Up @@ -239,6 +240,28 @@ public void Save()
this.editorOperations.SaveFile(this.scriptFile.FilePath);
}

/// <summary>
/// Save this file under a new path and open a new editor window on that file.
/// </summary>
/// <param name="newFilePath">
/// the path where the file should be saved,
/// including the file name with extension as the leaf
/// </param>
public void SaveAs(string newFilePath)
{
// Do some validation here so that we can provide a helpful error if the path won't work
string absolutePath = System.IO.Path.IsPathRooted(newFilePath) ?
newFilePath :
System.IO.Path.GetFullPath(System.IO.Path.Combine(System.IO.Path.GetDirectoryName(this.scriptFile.FilePath), newFilePath));

if (File.Exists(absolutePath))
{
throw new IOException(String.Format("The file '{0}' already exists", absolutePath));
Copy link
Member

Choose a reason for hiding this comment

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

Does this crash pses or does it just throw an error in the PSIC? (hoping for the second)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the second on the basis that it's a method on the $psEditor variable rather than in PSES itself (because I remember asking myself the same thing), but will check.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense :)

}

this.editorOperations.SaveFile(this.scriptFile.FilePath, newFilePath);
}

#endregion
}
}
Expand Down
8 changes: 8 additions & 0 deletions src/PowerShellEditorServices/Extensions/IEditorOperations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,14 @@ public interface IEditorOperations
/// <returns>A Task that can be tracked for completion.</returns>
Task SaveFile(string filePath);

/// <summary>
/// Causes a file to be saved as a new file in a new editor window.
/// </summary>
/// <param name="oldFilePath">the path of the current file being saved</param>
/// <param name="newFilePath">the path of the new file where the current window content will be saved</param>
/// <returns></returns>
Task SaveFile(string oldFilePath, string newFilePath);

/// <summary>
/// Inserts text into the specified range for the file at the specified path.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ await this.extensionEventQueue.EnqueueAsync(

public class TestEditorOperations : IEditorOperations
{

public string GetWorkspacePath()
{
throw new NotImplementedException();
Expand Down Expand Up @@ -206,6 +206,11 @@ public Task CloseFile(string filePath)
}

public Task SaveFile(string filePath)
{
return SaveFile(filePath, null);
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we could write a test for this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have tests for the editor method tasks? I thought I'd route through the methods on the mocks, but can happily add to tests.

Copy link
Member

Choose a reason for hiding this comment

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

The only reason I was wondering was because this change was in a test file (test/PowerShellEditorServices.Test/Extensions/ExtensionServiceTests.cs).

Copy link
Member

Choose a reason for hiding this comment

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

I don't actually know :/

Copy link
Member

Choose a reason for hiding this comment

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

looks like it's just mocks. No actual tests.

}

public Task SaveFile(string filePath, string newSavePath)
{
throw new NotImplementedException();
}
Expand Down