Add automatic colons or paranthesis to nodes when needed #2112
Conversation
There was a problem hiding this comment.
Great work on this! I took the liberty of adding a few questions and suggestions—I hope that's alright with you.
I saw in the description that you're still considering whether this will be the final PR or if additional initializers are coming. If you're leaning towards making this the final one, don't forget to run swift-format after your latest changes.
natikgadzhi
force-pushed
the
syntax/enum-case-parameter
branch
from
852e890
to
bdbc91b
Compare
|
@Matejkob HUGE thank you for the review!
Formatting aside, I will try and work on a few more items on the list above. I'll try to keep this PR small — if you'd prefer to merge this work in small chunks, I would love that as well. |
natikgadzhi
force-pushed
the
syntax/enum-case-parameter
branch
from
bdbc91b
to
e426182
Compare
Which error are you seeing? Running |
| @@ -45,3 +45,57 @@ extension MemberAccessExprSyntax { | |||
| ) | |||
| } | |||
| } | |||
|
|
|||
| extension EnumCaseParameterSyntax { | |||
There was a problem hiding this comment.
Could you order these extensions alphabetically and add the following section at the end of the file, similar to what we have in other parts of the codebase.
//==========================================================================//
// IMPORTANT: If you are tempted to add an extension here, please insert //
// it in alphabetical order above //
//==========================================================================//There was a problem hiding this comment.
I think that writing a script that does this automatically for an arbitrary file would be a nice exercise and an example of how to use SwiftSyntax!
How would you feel about having an additional script that ./format.py would invoke? It could live as an executable target in swift-format as well, perhaps?
Or.. maybe that could be a part of swift-format that would only trigger on files with paths matching a setting? I.e.
{
// ...
"rules" : {
"AllPublicDeclarationsHaveDocumentation" : false,
"AlwaysUseLowerCamelCase" : true,
// What if a rule would support not only a Boolean value, but an array of paths for which the value is true (or different from a default value)
"AlphabetizeDeclarations": [ "Sources/SwiftSyntax/generated/Convenience.swift", "**/Convenience.swift"]
// ...
},
// ...
}
I could start with a little experiment as an example to learn to use SwiftSyntax, and if I can get a prototype, I could write a proposal on Swift Forums to make the settings support path arrays?
There was a problem hiding this comment.
We discussed a similar idea here. At the time, we concluded that it wouldn't be a trivial task, so the subject was put on the back burner. However, the fact that the idea has resurfaced suggests there might be a real need for such a tool, and it could be beneficial to revisit the discussion.
How about we move this discussion to a separate issue? @natikgadzhi, would you like to do the honors and open one?
There was a problem hiding this comment.
If you have an idea of how to implement it, I’d be happy to take it. But as I mentioned in the other discussion, I think it’s far from a trivial task, if not even (nearly) impossible.
And so far, we have been good with these comments so at least as far as I’m concerned there’s no immediate need for action.
natikgadzhi
force-pushed
the
syntax/enum-case-parameter
branch
from
e426182
to
954c2bc
Compare
I cooked SwiftPM wrong, and it couldn't reset @Matejkob @ahoppen thank you both for reviewing, and for your feedback! I'm learning the pieces, and it starts to make more sense with every hour 🙃 I worked through the feedback and suggestions. Since we're not squashing commits, I've amended the original one, so it looks clean in the repo history. I will add the next items on the list as separate commits. I have a suggestion: @ahoppen, @Matejkob, how do you feel about splitting the work into three pull requests:
That way, I can make progress on them and ship them one by one faster than holding a bigger PR. Hopefully, it will be easier to review as well. |
|
@swift-ci Please test |
I’m also happy to merge this one as-is as a proof of concept and then make any similar changes to other nodes in follow-up PRs. But you can also keep amending this PR, whatever you prefer. |
|
@swift-ci Please test Windows |
Yep, makes sense — happy to work on the rest of it. |
|
Great. Thanks |
Provides release notes entries for: - `ClosureCaptureSyntax.init` apple#2127 - `EnumCaseParameterSyntax.init` apple#2112
Provides release notes entries for: - `ClosureCaptureSyntax.init` apple#2127 - `EnumCaseParameterSyntax.init` apple#2112 Co-authored-by: Mateusz Bąk <bakmatthew@icloud.com>
Provides release notes entries for: - `ClosureCaptureSyntax.init` apple#2127 - `EnumCaseParameterSyntax.init` apple#2112 Co-authored-by: Mateusz Bąk <bakmatthew@icloud.com>
This pull request adds a convenience init to
EnumCaseParameterSyntaxso that when you're initializing it with afirstName, it addscolonautomatically.It's the first step in #1984.
Changes
SwiftSyntaxalready had aConvenience.swift, so I'm adding an extension there for now. The file is small and readable.SwiftSyntaxTest/SyntaxTest.swift, however as we progress in Add initializer toLabeledExprSyntaxthat automatically adds a colon if the label exists #1984, I'll perhaps extract them into aConvenienceInitTests.swift.How to review this
let firstName: TokenSyntax? = firstNameto trick Swift into using the right overload with an optionalfirstNameso that the underlying generatedinitwill be called, instead of copying it over.TODO
Copying over from the issue:
And the same strategy can also be applied
ClosureCaptureSyntax.equalAttributeSyntaxMacroExpansionDeclSyntaxClosureCaptureModifierSyntaxFunctionCallExprSyntaxFreestandingMacroExpansionSyntax