Skip to content

Make Set-ScriptExtent not slow #1981

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

Conversation

SeeminglyScience
Copy link
Collaborator

This was some of the first code I ever contributed...and it shows. It's actually really easy to solve the problem of "when editing many parts of the same document, offsets no longer match up". You just go in reverse, and then you don't have that problem anymore.

So this gets rid of the "waiting for the document" to update which takes forever.

When I first wrote this many moons ago I didn't know most things. Like
that you can just edit in reverse and side step all positioning
issues...

This gets rid of the awful "wait for document update" logic and just
does it properly.
@SeeminglyScience SeeminglyScience requested a review from a team as a code owner December 28, 2022 21:03
)
begin {
$fileContext = $psEditor.GetEditorContext().CurrentFile
$extentList = [System.Collections.Generic.List[[Microsoft.PowerShell.EditorServices.Extensions.FileScriptExtent, Microsoft.PowerShell.EditorServices]]]::new()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before this point is all style fixes

end {
$needsIndentFix = $false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Defensively assigning to avoid potentially getting a variable with the same name from a parent scope

Comment on lines -39 to -40
# Currently this kills the pipeline because we need to keep track and edit all extents for position tracking.
# TODO: Consider queueing changes in a static property and adding a PassThru switch.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't kill the pipeline, it just stops streaming behavior. Doesn't really need a comment.

Also the TODO doesn't make any sense

$indentOffset = ' ' * ($aExtent.StartColumnNumber - 1)
$aText = $aText -split '\r?\n' `
-join ([Environment]::NewLine + $indentOffset)
$aText = $aText -split '\r?\n' -join ([Environment]::NewLine + $indentOffset)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just formatting. The rest of this should probably be revisited at some point as well, but not actively bad afaik

Copy link
Member

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

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

Nice work.

@andyleejordan andyleejordan added Issue-Enhancement A feature request (enhancement). Area-API labels Jan 3, 2023
@andyleejordan andyleejordan merged commit c1733a1 into PowerShell:main Jan 3, 2023
@SeeminglyScience SeeminglyScience deleted the make-set-scriptextent-faster branch January 3, 2023 19:39

[Parameter(Mandatory, ParameterSetName='AsString')]
[Parameter(Mandatory, ParameterSetName = 'AsString')]
[switch]
$AsString,
Copy link

Choose a reason for hiding this comment

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

Is it intentional that $AsString, is still kept on its own line here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nah not intentional, just missed it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-API Issue-Enhancement A feature request (enhancement).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants