Skip to content

Refactored file generation logic to be string-based and added validation tests #1047

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Refactored file generation logic to be string-based and added validation tests #1047

wants to merge 1 commit into from

Conversation

TTOzzi
Copy link
Member

@TTOzzi TTOzzi commented Jul 17, 2025

Resolve #1045

Changed the file generation logic to be string-based and added validation tests for each generated file.
Confirmed that it behaves the same as before 🫡

@TTOzzi TTOzzi force-pushed the generated-file-validation branch 2 times, most recently from a48a3d4 to 54b607f Compare July 17, 2025 13:55
@TTOzzi
Copy link
Member Author

TTOzzi commented Jul 17, 2025 •
edited
Loading

Hmm... It seems that string handling behaves a bit differently on Windows 🤔
It results in a linker error on versions below 6.2, and test case failures on 6.2.
I'll check it out.

@TTOzzi TTOzzi force-pushed the generated-file-validation branch from 54b607f to ec3fb2b Compare July 17, 2025 14:48
@TTOzzi
Copy link
Member Author

TTOzzi commented Jul 17, 2025

To resolve the linker error caused by adding the executableTarget as a dependency of the testTarget, I separated the execution logic of generate-swift-format from its core.

I'm still investigating the test case failures on Windows.

@TTOzzi TTOzzi force-pushed the generated-file-validation branch 2 times, most recently from 97185d5 to 1f62c5d Compare July 17, 2025 15:26
@TTOzzi
Copy link
Member Author

TTOzzi commented Jul 17, 2025

The test failure was caused by String(contentsOf:) treating newlines as \r\n when reading existing files on Windows 😵‍💫
I resolved it by normalizing \r\n to \n.

@TTOzzi TTOzzi force-pushed the generated-file-validation branch from 1f62c5d to 58c01a2 Compare July 17, 2025 15:55
allevato
allevato reviewed Jul 17, 2025
View reviewed changes
Package.swift Outdated
@@ -58,6 +58,11 @@ var targets: [Target] = [
"SwiftOperators", "SwiftParser", "SwiftParserDiagnostics", "SwiftSyntax", "SwiftSyntaxBuilder",
])
),
.target(
name: "GenerateSwiftFormat",
Copy link
Member

Choose a reason for hiding this comment

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

Let's name this (and the directory) _GenerateSwiftFormat, as a signal to users that this target shouldn't be used directly by anyone.

Package.swift Outdated
"SwiftFormat"
]
dependencies: ["GenerateSwiftFormat"],
path: "Sources/generate-swift-format"
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, it can be omitted. I've also incorporated the remaining changes. Thanks!

Tests/SwiftFormatTests/Utilities/GeneratedFilesValidityTests.swift Outdated
XCTAssertEqual(
generated,
fileContents.normalizeNewlines(),
"Pipelines+Generated.swift is out of date. Run generate-swift-format."
Copy link
Member

Choose a reason for hiding this comment

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

Let's give the full command to make it easier for contributors:

Whatever.swift is out of date. Please run 'swift run generate-swift-format'.

@TTOzzi TTOzzi force-pushed the generated-file-validation branch from 58c01a2 to 068b561 Compare July 18, 2025 14:51
@TTOzzi TTOzzi requested a review from allevato July 18, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

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

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

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

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

Add generated code validation step to CI
2 participants
@TTOzzi @allevato