Skip to content

Fix New-EditorFile failing when no Editor window open #1411

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

Merged
merged 3 commits into from
Mar 26, 2021
Merged

Fix New-EditorFile failing when no Editor window open #1411

merged 3 commits into from
Mar 26, 2021

Conversation

corbob
Copy link
Contributor

@corbob corbob commented Feb 13, 2021

Fixes PowerShell/vscode-powershell#3180

This feels janky, but short of moving the context gathering later and doing a bunch of work we can't complete because of Temp session I'm not sure of a better way.

I've also updated the help to reference the correct filename.

@corbob corbob requested a review from rjmholt as a code owner February 13, 2021 04:07
}
catch {
# If there's no editor, this throws an error. Create a new file, and grab the context here.
# This feels really hacky way to do it, but not sure if there's another way to detect editor context...
Copy link
Member

Choose a reason for hiding this comment

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

This definitely appears like it would work, so I don't mind the approach. I would like to know if there's a way to test if an editor context exists (if so we can avoid exception handling), and if creating a new file on the workspace is the best way to generate a new editor context. If this is the only approach, then I feel like our interface is lacking because as someone new to the code, I would have expected something like:

  • GetEditorContext() (which exists)
  • HasEditorContext() (essentially says if the above would throw)
  • NewEditorContext() (to create one, instead of relying what I presume is a side-effect of Workspace.NewFile())

@rjmholt
Copy link
Contributor

rjmholt commented Feb 16, 2021

Ah ok so @andschwa's comment has reminded me of the deeper nature of this API, and that it's essentially an old API that had to undergo some changes when we migrated to Omnisharp.

Here's the actual implementation of the method that gets called:

public async Task<EditorContext> GetEditorContextAsync()
{
if (!TestHasLanguageServer())
{
return null;
};
ClientEditorContext clientContext =
await _languageServer.SendRequest<GetEditorContextRequest>(
"editor/getEditorContext",
new GetEditorContextRequest())
.Returning<ClientEditorContext>(CancellationToken.None)
.ConfigureAwait(false);
return this.ConvertClientEditorContext(clientContext);
}

Some things we should look at here are:

  • The full error message
  • Whether we should change the synchronous call from using .Result to .GetAwaiter().GetResult() (I think so)
  • How we could update this API to make this scenario more seamless

@rjmholt
Copy link
Contributor

rjmholt commented Feb 16, 2021

@corbob what does the full error message look like here?

@rjmholt
Copy link
Contributor

rjmholt commented Feb 16, 2021

In particular wrt @andschwa's proposal, my counter proposal would just be for GetEditorContext() to create a new context if need be (or return null if there's no language server)

…e.ps1

Co-authored-by: Robert Holt <rjmholt@gmail.com>
@corbob
Copy link
Contributor Author

corbob commented Feb 17, 2021

@corbob what does the full error message look like here?

Here's a screenshot for color coding, followed by the copy/paste of the error:

image

┏[corbob]
┖[E:]> $pseditor.GetEditorContext()

MethodInvocationException: Exception calling "GetEditorContext" with "0" argument(s): "One or more errors occurred. (Internal error. )"
┏[corbob]
┖[E:]> $Error| select *


PSMessageDetails      :
Exception             : System.Management.Automation.MethodInvocationException: Exception calling "GetEditorContext" with "0" argument(s): "One or more errors occurred. (Internal error. )"
                         ---> System.AggregateException: One or more errors occurred. (Internal error. )
                         ---> OmniSharp.Extensions.JsonRpc.Server.InternalErrorException: Internal error.
                           at OmniSharp.Extensions.JsonRpc.ResponseRouter.ResponseRouterReturnsImpl.Returning[TResponse](CancellationToken cancellationToken)
                           at Microsoft.PowerShell.EditorServices.Services.EditorOperationsService.GetEditorContextAsync() in
                        D:\a\1\s\src\PowerShellEditorServices\Services\PowerShellContext\EditorOperationsService.cs:line 41
                           --- End of inner exception stack trace ---
                           at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)
                           at CallSite.Target(Closure , CallSite , Object )
                           --- End of inner exception stack trace ---
                           at System.Management.Automation.ExceptionHandlingOps.ConvertToMethodInvocationException(Exception exception, Type typeToThrow, String methodName, Int32 numArgs, MemberInfo 
                        memberInfo)
                           at CallSite.Target(Closure , CallSite , Object )
                           at System.Dynamic.UpdateDelegates.UpdateAndExecute1[T0,TRet](CallSite site, T0 arg0)
                           at System.Management.Automation.Interpreter.DynamicInstruction`2.Run(InterpretedFrame frame)
                           at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame frame)
TargetObject          :
CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
FullyQualifiedErrorId : AggregateException
ErrorDetails          :
InvocationInfo        : System.Management.Automation.InvocationInfo
ScriptStackTrace      : at <ScriptBlock>, <No file>: line 1
PipelineIterationInfo : {}

@rjmholt
Copy link
Contributor

rjmholt commented Feb 17, 2021

---> OmniSharp.Extensions.JsonRpc.Server.InternalErrorException: Internal error.

Well that's not very helpful.

Ok my proposal for now is that we move the logic in the script inside the API call so that the file is created internally to it.

Also we should change the call that throws to use .GetAwaiter().GetResult() rather than .Result

@andyleejordan
Copy link
Member

Thanks @corbob!

@andyleejordan andyleejordan merged commit 4fa0346 into PowerShell:master Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants