Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Snippet edit feature #1343

Merged
merged 2 commits into from
Jan 17, 2024
Merged

Snippet edit feature #1343

merged 2 commits into from
Jan 17, 2024

Conversation

MariaSolOs
Copy link
Contributor

@MariaSolOs MariaSolOs commented Oct 26, 2023 •
edited
Loading

Adding support for snippet workspace edits.

Closes #724

@MariaSolOs
Copy link
Contributor Author

@dbaeumer while working on this I realized that it would be quite convenient to use StringValues in TextEdits to achieve this instead of introducing a new SnippetTextEdit type. That is, changing the type of textEdit.newText to string | StringValue.

The problem with that is that StringValue is still part of the the proposed inline completion feature, and I'm not sure about adding the dependence between the features.

@dbaeumer
Copy link
Member

@MariaSolOs I think we should stay away from changing TextEdit. TextEdit is used in many places and not all of them might support snippets. And it will be a big breaking change if clients use TextEdits as results with structural typing. The TS compiler will then emit errors since a StringValue will not be assignable to a string.

@MariaSolOs MariaSolOs marked this pull request as ready for review December 15, 2023 21:11
@MariaSolOs MariaSolOs force-pushed the snippet-edits branch 3 times, most recently from 1b2004f to ca399ea Compare December 15, 2023 22:13
dbaeumer
dbaeumer requested changes Dec 18, 2023
View reviewed changes
protocol/src/common/protocol.ts Show resolved Hide resolved
types/src/main.ts Outdated Show resolved Hide resolved
@dbaeumer
Copy link
Member

As always great work.

@MariaSolOs
Copy link
Contributor Author

@dbaeumer I've addressed your comments. Please let me know if I'm missing anything else :)

dbaeumer
dbaeumer reviewed Jan 8, 2024
View reviewed changes
Copy link
Member

@dbaeumer dbaeumer left a comment

Choose a reason for hiding this comment

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

As always very good work

types/src/main.ts Outdated Show resolved Hide resolved
@vscodenpa vscodenpa added this to the December / January 2024 milestone Jan 17, 2024
@dbaeumer dbaeumer merged commit 2c7e623 into microsoft:main Jan 17, 2024
6 checks passed
@dbaeumer
Copy link
Member

@MariaSolOs will you provide a PR for the spec as well?

@rchl rchl mentioned this pull request Jan 17, 2024
Closed
@MariaSolOs MariaSolOs deleted the snippet-edits branch January 17, 2024 17:35
@MariaSolOs
Copy link
Contributor Author

MariaSolOs commented Jan 17, 2024 •
edited
Loading

@MariaSolOs will you provide a PR for the spec as well?

Yep, working on it.

EDIT: I will wait for the resolution of #1343 (comment) before updating the specs.

@MariaSolOs MariaSolOs mentioned this pull request Jan 17, 2024
Merged
@MariaSolOs MariaSolOs mentioned this pull request Feb 1, 2024
Merged
@MariaSolOs MariaSolOs mentioned this pull request Jun 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rchl rchl rchl left review comments

@DanTup DanTup DanTup left review comments

@dbaeumer dbaeumer dbaeumer approved these changes

@alexr00 alexr00 alexr00 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow CodeActions to specify cursor position
6 participants
@MariaSolOs @dbaeumer @rchl @DanTup @alexr00 @vscodenpa