Skip to content

Add more logging and internal documentation - #1474

Merged
Andy Jordan (andyleejordan) merged 5 commits into
masterfrom
andschwa/comments
May 19, 2021
Merged

Add more logging and internal documentation#1474
Andy Jordan (andyleejordan) merged 5 commits into
masterfrom
andschwa/comments

Conversation

@andyleejordan

@andyleejordanAndy Jordan (andyleejordan) commented May 11, 2021•
edited
Loading

Copy link
Copy Markdown
Member

Just writing up what I've learned and odd stuff I've spotted while reading through the code. Also added at least a bit more logging to help debug PowerShell/vscode-powershell#3306.

rjmholt
Rob Holt (rjmholt) reviewed May 11, 2021
View reviewed changes
Comment threadsrc/PowerShellEditorServices.Hosting/EditorServicesLoader.cs
Comment threadsrc/PowerShellEditorServices.Hosting/Internal/EditorServicesRunner.cs
Comment threadsrc/PowerShellEditorServices.Hosting/Internal/EditorServicesRunner.cs Outdated
Comment threadsrc/PowerShellEditorServices/Server/PsesServiceCollectionExtensions.cs Outdated
andyleejordan
Andy Jordan (andyleejordan) commented May 13, 2021
View reviewed changes
Comment threadsrc/PowerShellEditorServices.Hosting/Internal/EditorServicesRunner.cs
/// cref="HostStartupInfo"/>.
/// </summary>
/// <remarks>
/// The logical stack of the program is:

@andyleejordanAndy Jordan (andyleejordan)May 13, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Rob Holt (@rjmholt) Did I get this right?

@rjmholtRob Holt (rjmholt)May 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah I think that looks right

@andyleejordanAndy Jordan (andyleejordan)May 13, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Thanks! This was super useful (to me) to work through and document.

@andyleejordanAndy Jordan (andyleejordan) changed the title WIP: Commenting and loggingAdd more logging and internal documentationMay 13, 2021
@andyleejordan
Andy Jordan (andyleejordan) marked this pull request as ready for review May 13, 2021 01:41
This was referenced May 13, 2021
Closed
Open
Closed
Closed
Open
rjmholt
Rob Holt (rjmholt) reviewed May 13, 2021
View reviewed changes
Comment threadsrc/PowerShellEditorServices.Hosting/Configuration/HostLogger.cs
Comment threadsrc/PowerShellEditorServices.Hosting/Internal/EditorServicesRunner.cs
Comment threadsrc/PowerShellEditorServices.Hosting/Internal/EditorServicesRunner.cs
/// cref="HostStartupInfo"/>.
/// </summary>
/// <remarks>
/// The logical stack of the program is:

@rjmholtRob Holt (rjmholt)May 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah I think that looks right

Comment threadsrc/PowerShellEditorServices/Hosting/EditorServicesServerFactory.cs
Comment threadsrc/PowerShellEditorServices/Hosting/HostStartupInfo.cs
Comment on lines +170 to +171
/// This is a strange class that is generally <c>null</c> or otherwise just has a single path
/// set. It is eventually parsed one-by-one when setting up the PowerShell runspace.

@rjmholtRob Holt (rjmholt)May 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This class is used for configuring profile path values, which are set by the client. Different clients conceptually implement different hosts (so VSCode implements a different host to vim or IntelliJ), so they need different profile paths. This is how they configure that.

Comment threadsrc/PowerShellEditorServices/Server/PsesLanguageServer.cs
Comment threadsrc/PowerShellEditorServices/Server/PsesServiceCollectionExtensions.cs
provider.GetService<OmniSharp.Extensions.LanguageServer.Protocol.Server.ILanguageServerFacade>(),
hostStartupInfo))
.AddSingleton<TemplateService>()
.AddSingleton<TemplateService>() // TODO: What's the difference between this and the TemplateHandler?

@rjmholtRob Holt (rjmholt)May 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Handlers handle requests from the LSP client, like ASP.NET controllers.

Services internally manage some conceptual area, and generally implement the business logic behind handlers.

@andyleejordanAndy Jordan (andyleejordan)May 13, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Helpful!

@andyleejordan
Andy Jordan (andyleejordan)force-pushed the andschwa/comments branch 2 times, most recently from eb3769c to dab4e3cCompareMay 13, 2021 19:34
@andyleejordan
Add more logging to PowerShellContextService.cs
43f5e1b
Also change handled exceptions to warning instead of error.
@andyleejordanAndy Jordan (andyleejordan) mentioned this pull request May 13, 2021
Closed
@andyleejordanAndy Jordan (andyleejordan) added Ignore Exclude from the changelog. Area-Logging Issue-Enhancement A feature request (enhancement). and removed Ignore Exclude from the changelog. labels May 19, 2021
@andyleejordan
Andy Jordan (andyleejordan) deleted the andschwa/comments branch May 19, 2021 17:02
CopilotAI mentioned this pull request Aug 6, 2026
Draft
5 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@rjmholtRob Holt (rjmholt)rjmholt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-LoggingIssue-EnhancementA feature request (enhancement).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@andyleejordan@rjmholt