Skip to content

Conversation

@kimdv
Copy link
Contributor

@kimdv kimdv commented Feb 8, 2022

No description provided.

@kimdv kimdv force-pushed the kimdv/add-protocol-test branch from 502da19 to 6dd0e74 Compare February 8, 2022 09:48
@kimdv
Copy link
Contributor Author

kimdv commented Feb 8, 2022

@swift-ci please test

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Some minor comments, otherwise LGTM.

let returnClause = ArrayType(elementType: "DeclSyntax")
let input = ParameterClause(parameterListBuilder: {
FunctionParameter(firstName: .identifier("format"), colon: .colon, type: "Format", trailingComma: .comma, attributesBuilder: {})
FunctionParameter(firstName: .identifier("leadingTrivia"), colon: .colon, type: OptionalType(wrappedType: "Trivia"), attributesBuilder: {})
Copy link
Member

Choose a reason for hiding this comment

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

Is the attributesBuilder argument necessary? Shouldn’t it be defaulted?

Copy link
Contributor Author

@kimdv kimdv Feb 8, 2022

Choose a reason for hiding this comment

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

It should yes.
But the problem is that we then get Ambiguous use of 'init'.

I think we should, somehow, solve this as there is many init methods that look like each other so we need to do things like this to avoid the error. It works now, but I think it makes some noice/cluttered code when using it.

This is a long shot, and I'm not sure if it is technically possible, but I'll try to explain.

It would be nice if we could remove this type of init methods, where the convenience init method only have builders as different as the non-convenience init.
Like the one here: https://github.com/apple/swift-syntax/blob/caa735a5a5c41f4fc1769e5c34bb2cde17157400/Sources/SwiftSyntaxBuilder/gyb_generated/BuildableNodes.swift#L5676-L5721

This will mean, in this case, that our AttributeListBuilder need to conform to ExpressibleAsAttributeList.
And then we could things like FunctionParameter(modifiers: { TokenSyntax.public }, firstName: .identifier("format"), colon: .colon, type: "Format", trailingComma: .comma) or
FunctionParameter(modifiers: TokenSyntax.public, firstName: .identifier("format"), colon: .colon, type: "Format", trailingComma: .comma).

I think we before have talked about that the @resultBuilders should conform to some of the protocols.

Copy link
Member

Choose a reason for hiding this comment

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

I see, makes sense 👍 I’m good with merging this as-is.

As to your other question:
If I understand you correctly, you are saying that you’d like to remove the second initializer listed in the code snippet and make AttributeListBuilder conform to ExpressibleAsAttributeList. In that case you expect to be able to use the AttributeListBuilder result builder wherever an ExpressibleAsAttributeList is expected, right?
If this is what you’re suggesting, I’m pretty sure it won’t work. I think the result builder needs to be declared using @AttributeListBuilder on the parameter or otherwise the type checker won’t try to apply it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! Thanks, than it is this way :)

Did you see my question here too:
#356 (comment)

@kimdv kimdv force-pushed the kimdv/add-protocol-test branch from 6dd0e74 to 5f01be6 Compare February 8, 2022 18:17
@kimdv
Copy link
Contributor Author

kimdv commented Feb 8, 2022

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Feb 8, 2022

Build failed
Swift Test OS X Platform
Git Sha - 6dd0e74

@kimdv
Copy link
Contributor Author

kimdv commented Feb 8, 2022

Another question @ahoppen.
Do you know any place where this is used?
Because it would be cool to see how it is used and what we can do to make this package even better!

I have only been playing around with it, but nothing serious yet.

@kimdv
Copy link
Contributor Author

kimdv commented Feb 9, 2022

@swift-ci please test linux

@kimdv
Copy link
Contributor Author

kimdv commented Feb 17, 2022

Pinging @ahoppen 😊

@ahoppen ahoppen merged commit c1d767e into swiftlang:main Feb 17, 2022
@kimdv kimdv deleted the kimdv/add-protocol-test branch February 17, 2022 11:54
@ahoppen
Copy link
Member

ahoppen commented Feb 17, 2022

Thanks for the ping, I missed your other comments.

Do you know any place where this is used?

Unfortunately, I don’t know if it’s used at all, but I agree that I think the syntax builder has matured enough that we could start using it for real and refine it based on real world usage. What do you think about the following

  • Let’s update the README.md to the new syntax and include instructions how to use it (that is, which dependencies to declare in SwiftPM etc.)
  • When we’ve got a good readme with a few compelling examples, make a Swift Forums post in the Source Tooling category, announcing SwiftSyntaxBuilder, asking people to try it and give feedback (we can use the SwiftSyntax component on JIRA for bugs). Since you are the reason that SwiftSyntaxBuilder exists in the current state at all, I think you should also have the honor of creating the blog post if you wish

What do you think?

@kimdv
Copy link
Contributor Author

kimdv commented Feb 17, 2022

That sounds like a good idea!
I will update the README, and make a draft post, that we then can post in the forums!

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.

3 participants