Skip to content

Have the ChangeTracker filter out edits that are no-ops#38123

Merged
DanielRosenwasser merged 6 commits into
masterfrom
returnNoEditsForNoOps
Apr 23, 2020
Merged

Have the ChangeTracker filter out edits that are no-ops#38123
DanielRosenwasser merged 6 commits into
masterfrom
returnNoEditsForNoOps

Conversation

@DanielRosenwasser
Copy link
Copy Markdown
Member

@DanielRosenwasser DanielRosenwasser commented Apr 22, 2020

Fixes #22912

sheetalkamat
sheetalkamat reviewed Apr 22, 2020
View reviewed changes
Comment thread src/services/organizeImports.ts Outdated
@DanielRosenwasser DanielRosenwasser changed the title Filter out edits that are no-ops in 'organize imports' Have the change-tracker filter out edits that are no-ops Apr 23, 2020
@DanielRosenwasser DanielRosenwasser changed the title Have the change-tracker filter out edits that are no-ops Have the ChangeTracker filter out edits that are no-ops Apr 23, 2020
amcasey
amcasey approved these changes Apr 23, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

I'm kind of embarrassed by how simple this turned out to be.

LGTM, except for my perf comment.

Comment thread src/services/textChanges.ts Outdated
sheetalkamat
sheetalkamat reviewed Apr 23, 2020
View reviewed changes
Comment thread src/services/textChanges.ts Outdated
@DanielRosenwasser
Copy link
Copy Markdown
Member Author

DanielRosenwasser commented Apr 23, 2020

Honestly, this is a great practice in premature optimization, but I feel better knowing it's not much worse off. Summary:

  • There's a fast path for pure additions, deletions, other general differences - those are always kept.
  • When we need to check the contents, we don't allocate or traverse the whole file.
  • We don't allocate ChangeRanges for non-applicable changes or for files if they're empty or useless.

@DanielRosenwasser DanielRosenwasser merged commit d7e437a into master Apr 23, 2020
@DanielRosenwasser DanielRosenwasser deleted the returnNoEditsForNoOps branch April 23, 2020 19:54
@DanielRosenwasser DanielRosenwasser mentioned this pull request Jul 30, 2020
Closed
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sheetalkamat sheetalkamat sheetalkamat approved these changes

@elibarzilay elibarzilay Awaiting requested review from elibarzilay

+1 more reviewer

@amcasey amcasey amcasey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@DanielRosenwasser DanielRosenwasser

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Organize imports always returns an edit, even when imports are already sorted

3 participants

@DanielRosenwasser @sheetalkamat @amcasey