-
Notifications
You must be signed in to change notification settings - Fork 125
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.
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
Remove swift-syntax/swift-format as generator dependencies #343
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't review this in depth but it broadly looks good. I actually think the new generated is a little bit more readable than the code which was previously generated.
Yeah I agree. Thanks for the review, yeah no need to read every line, it's all very mechanical changes except for the new writer API. |
FYI @mbrandonw @tib this should help avoid future dependency graph conflicts, and @MahdiBM @gwynne this should also noticably speed up both the build and execution time of the generator. |
This looks like a very nice improvement indeed, I like the fact that we won't need swift-syntax at all, quick question are you going to drop support for older Swift versions in a future release (prior 5.9)? I believe, I noticed some code that already requires 5.9 to compile, but correct me if I'm wrong. |
Yes, that's the plan so that 1.0 in December ships with 5.9 being the minimum supported version. After 1.0, we expect to support 5.9 for several years, similar to other foundational libraries in the ecosystem, such as SwiftNIO. |
Motivation
The swift-syntax/swift-format dependencies were only used to reformat the code before writing to disk, and significantly increased build and execution times, as well as leading to more dependency graph conflicts for our adopters. Turns out we can relatively easily remove the dependency, so doing that in this PR.
Modifications
Removed swift-format/swift-syntax as dependencies and replaced them with an improved version of
TextBasedRenderer
, which now adds some basic indentation, surprisingly getting us pretty close to the previous formatting.Result
No more swift-syntax/swift-format dependency, but the generated code still looks very reasonable, and build/execution times should get noticably faster.
Test Plan
Adapted unit tests, integration tests: snippet and file-based tests. All passing now.