-
Notifications
You must be signed in to change notification settings - Fork 223
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
// | ||
|
||
using System; | ||
using System.IO; | ||
using System.Linq; | ||
using System.Management.Automation.Language; | ||
|
||
|
@@ -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)); | ||
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. 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. Choose a reason for hiding this commentThe 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 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. makes sense :) |
||
} | ||
|
||
this.editorOperations.SaveFile(this.scriptFile.FilePath, newFilePath); | ||
} | ||
|
||
#endregion | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -174,7 +174,7 @@ await this.extensionEventQueue.EnqueueAsync( | |
|
||
public class TestEditorOperations : IEditorOperations | ||
{ | ||
|
||
public string GetWorkspacePath() | ||
{ | ||
throw new NotImplementedException(); | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean we could write a test for this :) 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. 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. Choose a reason for hiding this commentThe 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 ( 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. I don't actually know :/ 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. looks like it's just mocks. No actual tests. |
||
} | ||
|
||
public Task SaveFile(string filePath, string newSavePath) | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
|
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.
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.
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.
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.
unless there's a way to be backcompat friendly?
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.
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.
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.
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.
Yeah ok -- I think that's fair. Especially after all the people asking for
SaveAs()
.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.
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.
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 whetherSaveAs()
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.
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
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.
shudder