Add $psEditor.GetEditorContext().CurrentFile.SaveAs("Name") support #650
Conversation
rjmholt
changed the title
| @@ -206,6 +206,11 @@ public Task CloseFile(string filePath) | |||
| } | |||
|
|
|||
| public Task SaveFile(string filePath) | |||
| { | |||
| return SaveFile(filePath, null); | |||
There was a problem hiding this comment.
Does this mean we could write a test for this :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The only reason I was wondering was because this change was in a test file (test/PowerShellEditorServices.Test/Extensions/ExtensionServiceTests.cs).
There was a problem hiding this comment.
I don't actually know :/
There was a problem hiding this comment.
looks like it's just mocks. No actual tests.
|
|
||
| if (File.Exists(absolutePath)) | ||
| { | ||
| throw new IOException(String.Format("The file '{0}' already exists", absolutePath)); |
There was a problem hiding this comment.
Does this crash pses or does it just throw an error in the PSIC? (hoping for the second)
There was a problem hiding this comment.
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.
| RequestType<string, EditorCommandResponse, object, object>.Create("editor/saveFile"); | ||
| RequestType<SaveFileDetails, EditorCommandResponse, object, object> Type = | ||
| RequestType<SaveFileDetails, EditorCommandResponse, object, object>.Create("editor/saveFile"); | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
unless there's a way to be backcompat friendly?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
breaking changes this small aren't bad. We just need to make sure we let people know in the release notes.
There was a problem hiding this comment.
Yeah ok -- I think that's fair. Especially after all the people asking for SaveAs().
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"...
There was a problem hiding this comment.
That last paragraph is awesome. Reminds me of graphQL in a lot of ways
Adds a
SaveAs()method to theFileContextobject, which sends a message to language client to save the current file as something else, at a path relative to the current file.One note: there's a small amount of validation, just to not let you overwrite a file (since VSCode won't). The question is: should we do more validation, or none and instead get VSCode to send us back a success/failure message?
See vscode-powershell #1261 for the client side of this feature.