-
Notifications
You must be signed in to change notification settings - Fork 221
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
Adjustments to split FunctionParameterSyntax
into multiple nodes for function parameters, closure parameters and enum parameters
#495
Conversation
/// called. | ||
/// - forcesBreakBeforeRightParen: Whether a break should be required before the right paren | ||
/// when the right paren is on a different line than the corresponding left paren. | ||
private func arrangeEnumCaseParameterClause( |
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 duplicated this method for EnumCaseParameterClauseSyntax
. Alternatives would be
- Make this function take a
leftParen: TokenSyntax, isEmpty: Bool, rightParen: TokenSyntax
so the logic can be shared witharrangeParameterClause
- Create a protocol that offers the three parameters described in (1) and make both
ParameterClauseSyntax
andEnumCaseParameterClauseSyntax
conform to that
(2) seemed like quite an overkill to me and (1) seemed ugly as well. Let me know if you’ve got preferences @allevato.
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 fine; the function is small enough that duplicating it is probably better than overengineering a generic thing that would only be used in one extra place.
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.
That’s what I thought as well. But good to hear you agree with me.
/// called. | ||
/// - forcesBreakBeforeRightParen: Whether a break should be required before the right paren | ||
/// when the right paren is on a different line than the corresponding left paren. | ||
private func arrangeEnumCaseParameterClause( |
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 fine; the function is small enough that duplicating it is probably better than overengineering a generic thing that would only be used in one extra place.
2b62c75
to
0d22a27
Compare
@allevato I have needed to duplicate some more code to make sure closure and enum case parameters are correctly formatted. Could you take another look whether you think it makes sense to unify some of these methods? I still think that it might be too much overhead but you decide. |
I still think the duplication is small enough that it's fine to have it for now, especially since all of these param lists are subtly different. If I want to revisit it in the future I can do that as part of a larger refactoring, but no reason to block you on it. |
…r function parameters, closure parameters and enum parameters Companion of swiftlang/swift-syntax#1455
0d22a27
to
1ce24a9
Compare
Companion of swiftlang/swift-syntax#1455