Skip to content

Merge IncrementalEdit andSourceEdit #2604

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
Merged
merged 3 commits into from
May 13, 2024
Merged

Merge IncrementalEdit andSourceEdit #2604

merged 3 commits into from
May 13, 2024

Conversation

kimdv
Copy link
Contributor

@kimdv kimdv commented Apr 13, 2024

Resolves #2532

I'm not sure if this what you envisioned.

@kimdv kimdv requested review from ahoppen and bnbarham as code owners April 13, 2024 10:30
@kimdv kimdv self-assigned this Apr 13, 2024
ahoppen
ahoppen reviewed Apr 15, 2024
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thank you @kimdv!

A few comments about deprecations I think we could perform. If those are too wide-reaching, I think it would make sense to do them in a follow-up PR.

@kimdv kimdv changed the title Merge incrementaledit and sourceedit Merge IncrementalEdit andSourceEdit Apr 16, 2024
@kimdv kimdv force-pushed the kimdv/2532-merge-incrementaledit-and-sourceedit branch 9 times, most recently from 32fdeb0 to 3a4bb8b Compare April 16, 2024 20:45
@kimdv
Copy link
Contributor Author

kimdv commented Apr 16, 2024

@ahoppen do you mind do a quick review?
Just to ensure I'm on the right path before I start fixes deprecation warnings. There is a few 😅

ahoppen
ahoppen reviewed Apr 17, 2024
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looks good to me. I think it would be good to gather a list of all the deprecations + API-incompatible changes as well. And once this is finalized, I’d like to do another double-check about which API is broken and which is deprecated to make sure it matches the release notes.

@kimdv kimdv force-pushed the kimdv/2532-merge-incrementaledit-and-sourceedit branch 3 times, most recently from 01dce84 to be77960 Compare April 20, 2024 20:15
@kimdv kimdv requested a review from ahoppen April 20, 2024 20:17
ahoppen
ahoppen previously approved these changes Apr 22, 2024
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thank you. Looks good 👍🏽

@ahoppen ahoppen dismissed their stale review April 22, 2024 19:20

Oh, one think I forgot: Could you add an entry to the release notes?

@kimdv
Copy link
Contributor Author

kimdv commented Apr 23, 2024 •
edited
Loading

@ahoppen should this go into 600 or 601?

Added to 6.0 as I saw you added byte range to that.
I will cherry pick to 6.0 as soon this PR is merged

@kimdv kimdv force-pushed the kimdv/2532-merge-incrementaledit-and-sourceedit branch from be77960 to f8853c9 Compare April 23, 2024 08:35
@kimdv kimdv requested a review from ahoppen April 23, 2024 08:35
@ahoppen
Copy link
Member

ahoppen commented Apr 24, 2024 •
edited
Loading

Sorry for changing my mind, @kimdv but I think it would be safer to not include this in 6.0. Changing/deprecating API that’s not strictly necessary is too big a risk for its value.

hamishknight
hamishknight approved these changes Apr 24, 2024
View reviewed changes
Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

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

LGTM, just one question about replacementLength

@kimdv kimdv force-pushed the kimdv/2532-merge-incrementaledit-and-sourceedit branch from f8853c9 to cbfcd62 Compare April 24, 2024 13:39
@kimdv
Copy link
Contributor Author

kimdv commented Apr 24, 2024

Sorry for changing my mind, @kimdv but I think it would be safer to not include this in 6.0. Changing/deprecating API that’s not strictly necessary is too big a risk for its value.

@ahoppen I changed to 601 now

@kimdv
Copy link
Contributor Author

kimdv commented Apr 24, 2024

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/2532-merge-incrementaledit-and-sourceedit branch 2 times, most recently from e6d6ee4 to 70e4088 Compare April 30, 2024 06:46
@kimdv
Copy link
Contributor Author

kimdv commented Apr 30, 2024

@swift-ci please test

1 similar comment
@kimdv
Copy link
Contributor Author

kimdv commented Apr 30, 2024

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/2532-merge-incrementaledit-and-sourceedit branch from 70e4088 to df37bb1 Compare May 9, 2024 13:32
@kimdv kimdv force-pushed the kimdv/2532-merge-incrementaledit-and-sourceedit branch from df37bb1 to 0c04746 Compare May 9, 2024 13:33
@kimdv kimdv mentioned this pull request May 9, 2024
Merged
@kimdv
Copy link
Contributor Author

kimdv commented May 10, 2024

swiftlang/sourcekit-lsp#1250
@swift-ci please test

@kimdv
Copy link
Contributor Author

kimdv commented May 10, 2024

swiftlang/sourcekit-lsp#1250
@swift-ci please test Windows

@ahoppen ahoppen merged commit 84aed58 into swiftlang:main May 13, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnbarham bnbarham bnbarham approved these changes

@hamishknight hamishknight hamishknight approved these changes

@ahoppen ahoppen Awaiting requested review from ahoppen ahoppen is a code owner

Assignees

@kimdv kimdv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge IncrementalEdit and SourceEdit
4 participants
@kimdv @ahoppen @bnbarham @hamishknight