Skip to content
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

added support for array of output objects. #8

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

mkowalski87
Copy link
Contributor

Added support for array of output objects #5

Copy link
Contributor

@mmysliwiec mmysliwiec left a comment

Choose a reason for hiding this comment

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

This is a great piece of work but I think we should polish it a bit.

@@ -1,2 +1,5 @@
@attached(peer, names: arbitrary)
public macro Parametrize<T>(input: [T]) = #externalMacro(module: "XCTestParametrizedMacroMacros", type: "ParametrizeMacro")
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're introducing generic param names for Input and Output we could also change it for original single Input so change from T to I for clarity. But it can be also done in the future cleanup/refactor so moving on.

var outputValues: ArrayElementListSyntax? {
get throws {
guard let firstMacroArgument = firstAttribute?.arguments?.as(LabeledExprListSyntax.self) else {
throw ParametrizeMacroError.macroAttributeNotAnArray
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to generalize the error message for this. Right now it mentions input only so it should be something like input/output. Another possibility is to introduce separate dedicated errors for input and output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now I've changed only text to make the message more generic

@@ -6,6 +6,7 @@ enum ParametrizeMacroError: Error, CustomStringConvertible {
case functionInputParamTypeMissing
case functionBodyEmpty
case macroAttributeNotAnArray
case macroAttributeMismatchSizeInputOutputArray
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should simplify the naming here. It's related to my previous comment about using macroAttributeNotAnArray error.
It could be something like macroAttributeArraysMismatchSize. This way we will not be forced to rename it in the future when implementing a proposal from the #9.

@@ -270,4 +270,31 @@ final class SimpleValuesTests: XCTestCase {
macros: testMacros
)
}

func testParametrizeInputOutput_SingleObject() throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's rather single Int so we should change the test name for clarity.

Copy link
Contributor Author

@mkowalski87 mkowalski87 Nov 10, 2023

Choose a reason for hiding this comment

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

also removed one duplicated test from other class.

@mkowalski87 mkowalski87 merged commit 9634ce3 into PGSSoft:main Nov 10, 2023
1 check passed
@mkowalski87 mkowalski87 deleted the feature/output-values branch November 15, 2023 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants