New-EditorFile works on non-powershell untitled files #774

TylerLeonhardt · 2018-10-14T01:23:18Z

Partnered with: [vscode-powershell issue]

This fixes the problem where $psEditor.GetEditorContext() assumed that the file open was a real file on the file system OR a powershell untitled file. Since that isn't always the case, It would throw an exception:

Exception calling "GetEditorContext" with "0" argument(s): "One or more errors occurred. (Could not find file '/Users/tyler/Code/PowerShell/vscode/PowerShellEditorServices/untitled:Untitled-1'.)"
At /Users/tyler/.vscode-insiders/extensions/ms-vscode.powershell-1.9.0/modules/PowerShellEditorServices/Commands/Public/CmdletInterface.ps1:152 char:13
+             $psEditor.GetEditorContext().CurrentFile.InsertText(($val ...
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
+ FullyQualifiedErrorId : AggregateException

This also means that New-EditorFile -Value 'asdf' actually works when you don't have your default language mode set to PowerShell.

Now we try to get the file and if that throws a FileNotFound, we grab the ScriptFile from the FileBuffer

fixes #434

rkeithhill · 2018-10-14T23:21:54Z

src/PowerShellEditorServices/Workspace/Workspace.cs

@@ -101,6 +101,25 @@ public ScriptFile GetFile(string filePath)
            return scriptFile;
        }

+        /// <summary>
+        /// Tries to get an open file in the workspace. Returns true or false if it succeeds.


Minor nit - would probably phrase the end of the summary this way "Returns true if it succeeds, false otherwise.".

@tylerl0706

rkeithhill

LGTM

SeeminglyScience

❤️ this change. One suggestion and one nit

src/PowerShellEditorServices.Protocol/LanguageServer/EditorCommands.cs

SeeminglyScience · 2018-10-15T13:12:02Z

src/PowerShellEditorServices.Protocol/Server/LanguageServerEditorOperations.cs

+            ScriptFile scriptFile = null;
+            if (!this.editorSession.Workspace.TryGetFile(clientContext.CurrentFilePath, out scriptFile))
+            {
+                scriptFile = this.editorSession.Workspace.GetFileBuffer(clientContext.CurrentFilePath, clientContext.CurrentFileContent);


nit: Consider putting each arg on a new line

rjmholt · 2018-10-15T16:39:47Z

src/PowerShellEditorServices/Workspace/Workspace.cs

+                scriptFile = GetFile(filePath);
+                return true;
+            }
+            catch (FileNotFoundException)


I'm guessing there are other reasons for failure here, like in:

PowerShellEditorServices/src/PowerShellEditorServices/Language/LanguageService.cs

Lines 345 to 355 in 13f2107

ScriptFile scriptFile;

try

{

scriptFile = workspace.GetFile(file);

}

catch (Exception e) when (e is IOException

|| e is SecurityException

|| e is FileNotFoundException

|| e is DirectoryNotFoundException

|| e is PathTooLongException

|| e is UnauthorizedAccessException)

It looks like we need to consolidate all our file access into a single internal API.

I think we should leave GetFile(), move this large catch inside TryGetFile(), and reuse TryGetFile() in the LanguageService and DebugAdapter. It looks like the opposite consideration is in GetFile() itself:

PowerShellEditorServices/src/PowerShellEditorServices/Workspace/Workspace.cs

Lines 81 to 98 in 834ae3d

if (!this.workspaceFiles.TryGetValue(keyName, out scriptFile))

{

// This method allows FileNotFoundException to bubble up

// if the file isn't found.

using (FileStream fileStream = new FileStream(resolvedFilePath, FileMode.Open, FileAccess.Read))

using (StreamReader streamReader = new StreamReader(fileStream, Encoding.UTF8))

{

scriptFile =

new ScriptFile(

resolvedFilePath,

filePath,

streamReader,

this.powerShellVersion);

this.workspaceFiles.Add(keyName, scriptFile);

}

this.logger.Write(LogLevel.Verbose, "Opened file on disk: " + resolvedFilePath);

I want to call this out.

We are logging the exception as well in the current implementation. Typically I've not seen TryX functions give access to the underlying exception.

That said, I guess we could add an overload that will let you out the exception as well:

public bool TryGetFile(string filePath, out ScriptFile scriptFile, out Exception exception)

I assume we don't want to log the exception within the Try_ method? It would contain a full stack trace, but we could log it as a warning, since we used a Try_.

rjmholt

LGTM!

TylerLeonhardt · 2018-10-21T00:45:01Z

proof: http://recordit.co/XeLdWL2sjC

TylerLeonhardt · 2018-10-21T00:46:04Z

oo CI failure on docs that's nice (pls still review as soon as you can because of time change)

TylerLeonhardt · 2018-10-21T09:37:52Z

Ok this is ready for review

SeeminglyScience

Can't wait for this :) awesome job! Just one question

src/PowerShellEditorServices/Workspace/Workspace.cs

rjmholt

LGTM -- just need to look at @rkeithhill's comment

rjmholt · 2018-12-02T23:57:47Z

PowerShell/vscode-powershell#194

TylerLeonhardt requested a review from rjmholt as a code owner October 14, 2018 01:23

TylerLeonhardt mentioned this pull request Oct 14, 2018

New-EditorFile works on non-powershell untitled files PowerShell/vscode-powershell#1580

Merged

TylerLeonhardt requested review from SeeminglyScience and rkeithhill October 14, 2018 06:04

rkeithhill reviewed Oct 14, 2018

View reviewed changes

rkeithhill approved these changes Oct 14, 2018

View reviewed changes

SeeminglyScience approved these changes Oct 15, 2018

View reviewed changes

rjmholt approved these changes Oct 15, 2018

View reviewed changes

TylerLeonhardt added 2 commits October 16, 2018 21:27

New-EditorFile works on non-powershell untitled files

750f17d

address robs feedback

5807f08

TylerLeonhardt force-pushed the fix-new-editorfile branch from 3374d06 to 5807f08 Compare October 16, 2018 14:32

have TryGetFile log exception

fafcbd2

rjmholt approved these changes Oct 17, 2018

View reviewed changes

TylerLeonhardt added 2 commits October 18, 2018 18:05

params on newline

a4d120a

create a new ScriptFile and have the language be in FileContext

be5b707

xml docs

045ebf7

SeeminglyScience approved these changes Oct 21, 2018

View reviewed changes

src/PowerShellEditorServices/Workspace/Workspace.cs Show resolved Hide resolved

rjmholt approved these changes Oct 23, 2018

View reviewed changes

address Keith's feedback

75e58da

rjmholt merged commit d85a019 into PowerShell:master Oct 29, 2018

TylerLeonhardt deleted the fix-new-editorfile branch September 16, 2019 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New-EditorFile works on non-powershell untitled files #774

New-EditorFile works on non-powershell untitled files #774

TylerLeonhardt commented Oct 14, 2018 •

edited

Loading

rkeithhill Oct 14, 2018

rjmholt Oct 23, 2018

TylerLeonhardt Oct 24, 2018

rkeithhill left a comment

SeeminglyScience left a comment

SeeminglyScience Oct 15, 2018

rjmholt Oct 15, 2018

rjmholt Oct 15, 2018

TylerLeonhardt Oct 16, 2018

rjmholt Oct 16, 2018

rjmholt left a comment

TylerLeonhardt commented Oct 21, 2018

TylerLeonhardt commented Oct 21, 2018

TylerLeonhardt commented Oct 21, 2018

SeeminglyScience left a comment

rjmholt left a comment

rjmholt commented Dec 2, 2018

	ScriptFile scriptFile;
	try
	{
	scriptFile = workspace.GetFile(file);
	}
	catch (Exception e) when (e is IOException
	\|\| e is SecurityException
	\|\| e is FileNotFoundException
	\|\| e is DirectoryNotFoundException
	\|\| e is PathTooLongException
	\|\| e is UnauthorizedAccessException)

	if (!this.workspaceFiles.TryGetValue(keyName, out scriptFile))
	{
	// This method allows FileNotFoundException to bubble up
	// if the file isn't found.
	using (FileStream fileStream = new FileStream(resolvedFilePath, FileMode.Open, FileAccess.Read))
	using (StreamReader streamReader = new StreamReader(fileStream, Encoding.UTF8))
	{
	scriptFile =
	new ScriptFile(
	resolvedFilePath,
	filePath,
	streamReader,
	this.powerShellVersion);

	this.workspaceFiles.Add(keyName, scriptFile);
	}

	this.logger.Write(LogLevel.Verbose, "Opened file on disk: " + resolvedFilePath);

New-EditorFile works on non-powershell untitled files #774

New-EditorFile works on non-powershell untitled files #774

Conversation

TylerLeonhardt commented Oct 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill left a comment

Choose a reason for hiding this comment

SeeminglyScience 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

rjmholt left a comment

Choose a reason for hiding this comment

TylerLeonhardt commented Oct 21, 2018

TylerLeonhardt commented Oct 21, 2018

TylerLeonhardt commented Oct 21, 2018

SeeminglyScience left a comment

Choose a reason for hiding this comment

rjmholt left a comment

Choose a reason for hiding this comment

rjmholt commented Dec 2, 2018

TylerLeonhardt commented Oct 14, 2018 •

edited

Loading