Slightly clean up FixIt.Change and FixIt.Changes
#1460
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ahoppen
changed the title
FixIt.Change and FixIt.ChangesFixIt.Change and FixIt.Changes
|
@swift-ci Please test |
|
@swift-ci Please test |
kimdv
reviewed
Mar 29, 2023
kimdv
approved these changes
Mar 29, 2023
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
bnbarham
reviewed
Mar 29, 2023
| public func withPresence(_ newValue: SourcePresence, arena: SyntaxArena) -> RawSyntax { | ||
| switch raw.rawData.payload { | ||
| case .parsedToken(_): | ||
| // The wholeText can't be continuous anymore. Make a materialized token. |
There was a problem hiding this comment.
Why can't it be continuous any more? We're only changing presence here. It's true that we might then merge the trivia later (?) but that doesn't matter for here, am I missing something?
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
bnbarham
approved these changes
Mar 29, 2023
|
@swift-ci Please test Windows |
|
@swift-ci Please test |
|
@swift-ci Please test macOS |
|
@swift-ci Please test Windows |
|
@swift-ci Please test Linux |
|
@swift-ci Please test Windows |
The existing API with `FixIt.Change` and `FixIt.Changes` and the fact that we passed `[FixIt.Changes]` around what fairly weird. Ideally, we would only have `FixIt.Change` and make it powerful enough to represent all the trivia transfer that `FixIt.Changes` is doing. But given that that will be a larger effort, let’s make some smaller changes now that reduce the badness of the public API impact: - Move `FixIt.Changes` to `SwiftParserDiagnostics`, make it internal and rename it to `MultiNodeChange`. That way this type is no longer exposed in the public API, so we can iterate on it or remove it without breaking clients. The only thing that remains exposed is `FixIt.Change`, which we want to continue to support. We will probably add more cases to it in the future. - Make `FixIt.Changes` no longer conform to `ExpressibleByArrayLiteral`. That just makes everything a lot more explicit and easier to follow. - Remove some unecessary initializations of `FixIt.Changes` where `FixIt.Change` was sufficient. - Make `presence` on `TokenSyntax` settable so you can do `token.with(\.presence, .missing)` - Slightly unrelated: Rename SyntaxOtherNodes.swift to TokenSyntax.swift because that’s the only node it contains.
|
@swift-ci Please test |
|
@swift-ci Please test macOS |
|
@swift-ci Please test Windows |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The existing API with
FixIt.ChangeandFixIt.Changesand the fact that we passed[FixIt.Changes]around what fairly weird.Ideally, we would only have
FixIt.Changeand make it powerful enough to represent all the trivia transfer thatFixIt.Changesis doing. But given that that will be a larger effort, let’s make some smaller changes now that reduce the badness of the public API impact:FixIt.ChangestoSwiftParserDiagnostics, make it internal and rename it toMultiNodeChange. That way this type is no longer exposed in the public API, so we can iterate on it or remove it without breaking clients. The only thing that remains exposed isFixIt.Change, which we want to continue to support. We will probably add more cases to it in the future.FixIt.Changesno longer conform toExpressibleByArrayLiteral. That just makes everything a lot more explicit and easier to follow.FixIt.ChangeswhereFixIt.Changewas sufficient.presenceonTokenSyntaxsettable so you can dotoken.with(\.presence, .missing)