-
Notifications
You must be signed in to change notification settings - Fork 439
Add a public function that can be used to test macro expansions #1550
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
Conversation
7a16806
to
956ef25
Compare
@swift-ci Please test |
@swift-ci Please test macOS |
@swift-ci Please test Windows |
@swift-ci Please test macOS |
956ef25
to
d967132
Compare
@swift-ci Please test |
@swift-ci Please test Linux |
@swift-ci Please test Windows |
462ae24
to
8d1b64d
Compare
@swift-ci Please test |
@swift-ci Please test Windows |
@@ -56,6 +56,7 @@ let package = Package( | |||
.library(name: "SwiftSyntax", type: .static, targets: ["SwiftSyntax"]), | |||
.library(name: "SwiftSyntaxBuilder", type: .static, targets: ["SwiftSyntaxBuilder"]), | |||
.library(name: "SwiftSyntaxMacros", type: .static, targets: ["SwiftSyntaxMacros"]), | |||
.library(name: "SwiftSyntaxMacrosTestSupport", type: .static, targets: ["SwiftSyntaxMacrosTestSupport"]), |
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.
We should add this to the CMake build as well?
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 don’t think there’s any reason why this should be in the toolchain. So no need to add it to the CMake build…
import SwiftParser | ||
import SwiftSyntax | ||
import SwiftSyntaxMacros | ||
import XCTest |
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.
... or is this why we don't have it in the CMake build?
public func assertMacroExpansion( | ||
_ originalSource: String, | ||
expandedSource: String, | ||
diagnostics: [DiagnosticSpec] = [], | ||
macros: [String: Macro.Type], | ||
testModuleName: String = "TestModule", | ||
testFileName: String = "test.swift", | ||
file: StaticString = #file, | ||
line: UInt = #line |
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.
This is a really nice API 👍
8d1b64d
to
444648f
Compare
Note to self: This requires the BasicFormat improvements I’m merging in other PRs. |
444648f
to
996858b
Compare
Testing macros should be as easy as possible and we should thus make the `assertMacroExpansion` function public. While making the function public, I made the following changes: - Changed `diagnosticMessages` to expect `DiagnsoticSpec` similar to the parser tests. The only difference is that they take line and columns instead of location markers because the location markers have a slighgly high learning curve IMO. Slightly unrelated, I also made the following changes: - Made a few minor improvements to `SwiftParserTest/Assertions.swift` - Fixed a bug in MacroApplication where the wrong syntax node was passed as the `node` parameter` to `addDiagnostics`, leading to a highlight that was bigger than expected.
996858b
to
21654ab
Compare
21654ab
to
dd9c9e8
Compare
@swift-ci Please test |
@swift-ci Please test Windows |
|
||
private extension String { | ||
// This implementation is really slow; to use it outside a test it should be optimized. | ||
func trimmingTrailingWhitespace() -> String { |
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.
Do we need this if we've fixed BasicFormat?
) | ||
} | ||
if diag.notes.count != spec.notes.count { | ||
XCTFail( |
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.
Can we output both notes, ie. actual and expected? Same with fix-it's below.
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 don’t think it’s worth outputting the expected notes because the diagnostic will point right into the source that defines them. We used to output the expected diagnostics if the count did not match and I found that verbose without giving any value.
/// - testModuleName: The name of the test module to use. | ||
/// - testFileName: The name of the test file name to use. |
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.
Is it useful to expose these? It doesn't actually matter for anything right?
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.
You could use them in diagnostic messages, right?
macros: [String: Macro.Type], | ||
testModuleName: String = "TestModule", | ||
testFileName: String = "test.swift", | ||
indentationWidth: Trivia = .spaces(4), |
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.
What are your thoughts on indentWidth
vs indentationWidth
?
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 don’t have strong feelings. Do you?
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.
Define "strong". I prefer indentWidth
because it's shorter. But 🤷
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 just checked. SwiftSyntax general prefers indentation
over indent
, so I’d prefer to stick with indentation
for consistency.
@@ -721,51 +673,56 @@ public let testMacros: [String: Macro.Type] = [ | |||
] | |||
|
|||
final class MacroSystemTests: XCTestCase { | |||
private let indentationWidth: Trivia = .spaces(2) |
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.
static
?
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.
But then I need to access it with Self.indentationWidth
which is a little to verbose for my taste.
Testing macros should be as easy as possible and we should thus make the
assertMacroExpansion
function public.While making the function public, I made the following changes:
diagnosticMessages
to expectDiagnsoticSpec
similar to the parser tests. The only difference is that they take line and columns instead of location markers because the location markers have a slighgly high learning curve IMO.Slightly unrelated, I also made the following changes:
SwiftParserTest/Assertions.swift
node
parameterto
addDiagnostics`, leading to a highlight that was bigger than expected.