Skip to content

Add convert to trailing closure and editor placeholder refactorings #1485

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 4 commits into from
Jun 12, 2023
Merged

Add convert to trailing closure and editor placeholder refactorings #1485

merged 4 commits into from
Jun 12, 2023

Conversation

bnbarham
Copy link
Contributor

@bnbarham bnbarham commented Apr 2, 2023 •
edited
Loading

Adds three new refactorings:

  • ConvertToTrailingClosures
  • ExpandEditorPlaceholder
  • ExpandEditorPlaceholders

ExpandEditorPlaceholders is a combination of ExpandEditorPlaceholder and ConvertToTrailingClosures, ie. it first expands any function-typed closures at the end of a call using ExpandEditorPlaceholder and then runs ConvertToTrailingClosures on that call.

Resolves rdar://107532856.

@bnbarham bnbarham requested review from rintaro and hamishknight April 2, 2023 18:54
@bnbarham bnbarham requested a review from ahoppen as a code owner April 2, 2023 18:54
@bnbarham
Copy link
Contributor Author

bnbarham commented Apr 2, 2023

This has a couple of differences with the existing placeholder expansion:

  • Typed placeholders within strings are converted the same way as they would be outside of a string. They're currently expanded as is, ie. <#T##display##type#> would be expanded to T##display##type#.
  • Missing types in the type syntax would cause placeholders to be expanded as is. This now falls back as much as possible instead, see ExpandEditorPlaceholderTest.testEmpty. IMO this makes more sense, but happy to change if anyone feels strongly about it.

BasicFormat still needs some fixes here, but we can do that after. The main one is the missing space between } and the label for multiple trailing closures. There's also the indentation, but our current expectation is that clients would fix up indentation on any returned refactoring anyway.

bnbarham
bnbarham commented Apr 2, 2023
View reviewed changes
@bnbarham bnbarham force-pushed the placeholder-expansion branch 4 times, most recently from 4b05750 to ebfb348 Compare April 4, 2023 00:44
@bnbarham
Copy link
Contributor Author

bnbarham commented Apr 4, 2023

@swift-ci please test

ahoppen
ahoppen reviewed Apr 4, 2023
View reviewed changes
@DougGregor
Copy link
Member

@swift-ci please test Windows

@bnbarham bnbarham force-pushed the placeholder-expansion branch from ebfb348 to 4546b2e Compare April 9, 2023 17:30
@bnbarham bnbarham force-pushed the placeholder-expansion branch 2 times, most recently from 0fa4da4 to 38e564f Compare May 6, 2023 19:59
hamishknight
hamishknight approved these changes May 8, 2023
View reviewed changes
@bnbarham bnbarham force-pushed the placeholder-expansion branch from 38e564f to 2329889 Compare May 8, 2023 16:18
@bnbarham
Copy link
Contributor Author

bnbarham commented May 8, 2023

@swift-ci please test

@bnbarham
Copy link
Contributor Author

@swift-ci please test Windows platform

@bnbarham bnbarham requested a review from ahoppen May 17, 2023 23:42
ahoppen
ahoppen reviewed May 18, 2023
View reviewed changes
@bnbarham bnbarham force-pushed the placeholder-expansion branch from 2329889 to 303c058 Compare May 19, 2023 06:18
ahoppen
ahoppen reviewed May 19, 2023
View reviewed changes
Sources/SwiftRefactor/CallToTrailingClosures.swift Outdated
.with(\.leadingTrivia, Trivia()
.merging(triviaOf: closures.first!.original.label)
.merging(triviaOf: closures.first!.original.colon)
.merging(closures.first!.closure.leadingTrivia))
Copy link
Member

Choose a reason for hiding this comment

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

Are you dropping the trivia of closures.first!.trailingComma or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's added above. FWIW I added quite a few more comments in the trivia test that hopefully catches all the cases now 😅. Worth checking those as well - IMO there's a few odd spaces that we should probably remove, but I think it's good enough for now.

@bnbarham bnbarham force-pushed the placeholder-expansion branch 3 times, most recently from 312186b to 33a73d9 Compare May 20, 2023 03:43
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham
Copy link
Contributor Author

@swift-ci please test Windows platform

@bnbarham bnbarham force-pushed the placeholder-expansion branch from 33a73d9 to 7249399 Compare June 3, 2023 01:00
@bnbarham
Copy link
Contributor Author

bnbarham commented Jun 3, 2023

@swift-ci please test

@bnbarham
Copy link
Contributor Author

bnbarham commented Jun 5, 2023

@swift-ci please test

@bnbarham
Copy link
Contributor Author

bnbarham commented Jun 5, 2023

swiftlang/swift-stress-tester#238

@swift-ci please test

@bnbarham bnbarham mentioned this pull request Jun 5, 2023
Merged
@bnbarham bnbarham force-pushed the placeholder-expansion branch from 7249399 to 09c1295 Compare June 8, 2023 00:03
@bnbarham
Copy link
Contributor Author

bnbarham commented Jun 8, 2023

swiftlang/swift-stress-tester#238

@swift-ci please test

@bnbarham bnbarham force-pushed the placeholder-expansion branch 2 times, most recently from 0c9bcea to 37d87dc Compare June 8, 2023 18:57
@bnbarham
Copy link
Contributor Author

bnbarham commented Jun 8, 2023

swiftlang/swift-stress-tester#238

@swift-ci please test

bnbarham added 4 commits June 12, 2023 13:32
@bnbarham
BasicFormat should add spacing around { and }
195823b 
We now properly strip trailing whitespace. Add in spacing after `{` and
before `}`.
@bnbarham
Rename the incremental source edit to IncrementalEdit
40f2954 
This was called `SourceEdit`, which would be nice to reserve for
refactorings rather than incremental parsing. Ideally we'd also move all
the incremental parsing types into `SwiftParser` as well.
@bnbarham
Add an EditRefactoringProvider which provides textual edits
bcf0d1f 
This adds an additional text-based refactoring provider which the
original syntax-based provider now implements. This allows refactorings
that produce text rather than a full tree.
@bnbarham
Add convert to trailing closure and editor placeholder refactorings
c70e54a 
Adds three new refactorings:
  - `ConvertToTrailingClosures`
  - `ExpandEditorPlaceholder`
  - `ExpandEditorPlaceholders`

`ExpandEditorPlaceholders` is a combination of `ExpandEditorPlaceholder`
and `ConvertToTrailingClosures`, ie. it first expands any function-typed
closures at the end of a call using `ExpandEditorPlaceholder` and then
runs `ConvertToTrailingClosures` on that call.

Resolves rdar://107532856.
@bnbarham bnbarham force-pushed the placeholder-expansion branch from 37d87dc to c70e54a Compare June 12, 2023 20:33
@bnbarham
Copy link
Contributor Author

swiftlang/swift-stress-tester#238

@swift-ci please test

@bnbarham bnbarham merged commit 34453e6 into swiftlang:main Jun 12, 2023
@bnbarham bnbarham deleted the placeholder-expansion branch June 12, 2023 23:54
kimdv added a commit to kimdv/swift-syntax that referenced this pull request Jun 14, 2023
@kimdv
Fix code generation from swiftlang#1485
06603c1
@bnbarham bnbarham mentioned this pull request Jun 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen approved these changes

@hamishknight hamishknight hamishknight approved these changes

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

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

Successfully merging this pull request may close these issues.
4 participants
@bnbarham @DougGregor @ahoppen @hamishknight