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

Merge IncrementalEdit andSourceEdit #2604

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
Collaborator

@kimdv kimdv commented Apr 13, 2024

Resolves #2532

I'm not sure if this what you envisioned.

@kimdv kimdv self-assigned this Apr 13, 2024
ahoppen
ahoppen reviewed Apr 15, 2024
View reviewed changes
Copy link
Collaborator

@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.

Sources/SwiftSyntax/ByteSourceRange.swift Outdated Show resolved Hide resolved
Sources/SwiftSyntax/SourceEdit.swift Outdated Show resolved Hide resolved
Sources/SwiftSyntax/SourceEdit.swift Outdated Show resolved Hide resolved
Sources/SwiftSyntax/SourceEdit.swift Outdated Show resolved Hide resolved
Sources/SwiftSyntax/SourceEdit.swift Outdated Show resolved Hide resolved
Sources/SwiftSyntax/SourceEdit.swift Outdated Show resolved Hide resolved
@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
Collaborator 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
Collaborator

@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.

Sources/SwiftParser/IncrementalParseTransition.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/IncrementalParseTransition.swift Outdated Show resolved Hide resolved
Sources/SwiftParser/IncrementalParseTransition.swift Outdated Show resolved Hide resolved
Sources/SwiftSyntax/SourceEdit.swift Outdated Show resolved Hide resolved
Sources/SwiftSyntax/SourceEdit.swift Outdated Show resolved Hide resolved
Sources/SwiftSyntax/SourceEdit.swift Outdated Show resolved Hide resolved
Sources/SwiftSyntax/SourceEdit.swift Outdated Show resolved Hide resolved
@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
Collaborator

@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
Collaborator Author

kimdv commented Apr 23, 2024 •
edited

@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
Collaborator

ahoppen commented Apr 24, 2024 •
edited

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

Sources/SwiftSyntax/SourceEdit.swift Outdated Show resolved Hide resolved
@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
Collaborator 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
Collaborator 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
Collaborator Author

kimdv commented Apr 30, 2024

@swift-ci please test

1 similar comment
@kimdv
Copy link
Collaborator 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
Collaborator Author

kimdv commented May 10, 2024

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

@kimdv
Copy link
Collaborator Author

kimdv commented May 10, 2024

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

@ahoppen ahoppen merged commit 84aed58 into apple: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