-
Notifications
You must be signed in to change notification settings - Fork 234
add Save to FileContext API #590
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
Conversation
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.
LGTM but this is way more @daviwil area.
/// <param name="filePath">The path of the file to be saved.</param> | ||
/// <returns>A Task that can be tracked for completion.</returns> | ||
Task SaveFile(string filePath); | ||
|
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 adding to this interface be a breaking change? I know we have at least two new implementations of PSES since this interface was last changed. Should new features be added as additional interfaces, something like IEditorSaveOperations
?
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.. but I think if we were going to make a new interface, I'd want it to be IEditorFileOperations
and it would contain NewFile
, OpenFile
, CloseFile
, SaveFile`, ya know? And that would surely warrant 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.
As far as I know, nobody is using these interfaces outside of PSES itself. Adding a method to an interface wouldn't be a breaking change so I think a new interface shouldn't be necessary for this.
/// </summary> | ||
public void Save() | ||
{ | ||
this.editorOperations.SaveFile(this.scriptFile.FilePath); |
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.
If you pass ClientFilePath
here instead of FilePath
you should be able to avoid resolving the path when it gets to VSCode. The InsertText
method is a good example for both sides.
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.
@SeeminglyScience + @daviwil: So I tried passing in ClientFilePath
. It prefaced it with file://
.
This is not ideal because for openFile
and closeFile
here:
https://github.com/PowerShell/vscode-powershell/blob/master/src/features/ExtensionCommands.ts#L362-L409
We have VSCode already resolving the paths.
So I think we should either continue resolving the paths via VSCode, or modify openFile
and closeFile
to expect a ClientFilePath
as well. That way, we are consistent.
Thoughts?
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.
@tylerl0706 Ah I'd keep it as is then. In general I think it's better to have PSES do the work, but it's probably not worth changing the others.
/// <param name="filePath">The path of the file to be saved.</param> | ||
/// <returns>A Task that can be tracked for completion.</returns> | ||
Task SaveFile(string filePath); | ||
|
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.
As far as I know, nobody is using these interfaces outside of PSES itself. Adding a method to an interface wouldn't be a breaking change so I think a new interface shouldn't be necessary for this.
Thanks all! |
@tylerl0706 I think we missed a file here because I don't see the I think PowerShellEditorServices/src/PowerShellEditorServices/Extensions/EditorWorkspace.cs Lines 41 to 61 in 0f63e5c
BTW CloseFile() is missing as well. Hmm, maybe I'm not looking in the right place for this new SaveFile method. UPDATE: nevermind. Figured it out - $psEditor.GetEditorContext().CurrentFile.Save(). |
Add new integrated console settings
This is the PSES portion of this change. See the PSES portion here: PowerShell/vscode-powershell#1139
Please let me know if I'm missing something.
To test this, you can use:
$psEditor.GetEditorContext().CurrentFile.Save()
in the Integrated Console.