Conversation
a7maad-ayman
requested review from
ahoppen,
hamishknight,
hborla and
rintaro
as code owners
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Thanks for starting this, @a7maad-ayman. I left some initial comments from my first scan through the PR. Once those are resolved, I’ll have another more detailed look at it.
| /// - Optionally, the variable is declared immediately before the if statement | ||
| public struct ConvertToTernaryExpression: SyntaxRefactoringProvider { | ||
|
|
||
| public static func refactor(syntax: CodeBlockItemListSyntax, in context: Void) throws -> CodeBlockItemListSyntax { |
There was a problem hiding this comment.
Could you run swift-format on your changes. Instructions should be in CONTRIBUTING.md
| let items = Array(codeBlock) | ||
| guard !items.isEmpty else { return nil } | ||
|
|
||
| /// Variable declaration followed by if statement. |
There was a problem hiding this comment.
Would it be simpler to find the if expression to extract first and then just look up if the previous statement is a variable declaration of that name? I feel like that could unify this branch and the one below a little.
| private static func extractCondition(from ifExpr: IfExprSyntax) -> ExprSyntax? { | ||
| guard let firstCondition = ifExpr.conditions.first else { | ||
| return nil | ||
| } | ||
|
|
||
| guard case .expression(let condition) = firstCondition.condition else { | ||
| return nil | ||
| } | ||
|
|
||
| return condition | ||
| } | ||
|
|
There was a problem hiding this comment.
I think the entire code in analyzePattern would read easier if these checks would just be inline. This one, for example could just be a single
guard case .expression(let condition) = ifExpr.conditions.first.condition else {
return nil
}Similar for most of the other checks
| } | ||
|
|
||
| private static func extractCondition(from ifExpr: IfExprSyntax) -> ExprSyntax? { | ||
| guard let firstCondition = ifExpr.conditions.first else { |
There was a problem hiding this comment.
We should only apply this transformation if there’s only a single condition. You can use .only for that.
| guard codeBlock.statements.count == 1, | ||
| let statement = codeBlock.statements.first |
| let expectedElementCount = 3 | ||
| let assignmentOperatorIndex = 1 | ||
|
|
||
| guard elements.count == expectedElementCount, |
There was a problem hiding this comment.
Let’s just inline 3 here. The variable name doesn’t provide much value. Same for assignmentOperatorIndex.
| if expr.as(TernaryExprSyntax.self) != nil { return true } | ||
| if expr.as(ClosureExprSyntax.self) != nil { return true } |
There was a problem hiding this comment.
Why are these too complex for a ternary?
| // MARK: - Alternative API for single if statement refactoring | ||
| extension ConvertToTernaryExpression { | ||
|
|
||
| public static func refactor( |
There was a problem hiding this comment.
AFAICT this doesn’t satisfy any requirement in SyntaxRefactoringProvider. Since most clients will enter the refactoring action through that protocol, this method is practically unreachable. We should be able to perform this refactoring using the main refactor entry point.
| } | ||
|
|
||
| // MARK: - Builders | ||
| private static func withoutTrivia<T: SyntaxProtocol>(_ node: T) -> T { |
There was a problem hiding this comment.
We have .trimmed, which does exactly this.
|
|
||
| func testNamedTupleAssignment() throws { | ||
| let baseline: CodeBlockItemListSyntax = """ | ||
| let coordinates: (x: Int, y: Int) |
There was a problem hiding this comment.
Should we keep the type annotation here?
2 participants
Refactor ConvertToTernaryExpr
Implements a new Swift refactoring that converts if-else statements with assignments into ternary expressions.
What it does
Converts this:
Into this:
Supported patterns
(a, b) = condition ? (1, 2) : (3, 4)x = condition ? value1 : value2Contributes to #2424