Preserve async/throws when converting functions to computed properties #3235 #3237
Conversation
vishakha1411
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.
Could you also add a test case for this? Also, could you ensure that we only produce the explicit get accessor if there are effect specifiers? I believe the current tests should check that, did you run them?
|
I have added test cases. Also, he implementation now only generates an explicit get accessor when effect specifiers (async and/or throws) are present; otherwise, it continues to use the implicit getter as before. |
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Two high-level comments before I go into an in-depth review: Could you please ensure that your PR does not contain unnecessary formatting changes? Also, instead of hard-coding 2 spaces as the indentation could you use BasicFormat.inferIndentation to infer the indentation from the source file?
|
I updated the code to use BasicFormat.inferIndentation(of: syntax) so the refactoring follows the source file’s indentation. |
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Thanks for fixing this @vishakha1411 ❤️ I have one comment below, otherwise this looks good.
| let accessorBlock: AccessorBlockSyntax | ||
|
|
||
| if let accessorEffectSpecifiers { | ||
| let indentedStatements = body.statements.map { $0.with(\.leadingTrivia, $0.leadingTrivia + indentation) } |
There was a problem hiding this comment.
This will not indent multi-line statements, eg. the following. Could you use body.statements.indented which handles all of this?
func foo() async {
bar(
1
)
}Also, please add a test for this.
There was a problem hiding this comment.
I have replaced this with body.statements.indented and also added a couple of test cases for the same.
| accessors: .accessors( | ||
| AccessorDeclListSyntax([getAccessor]) | ||
| ), |
There was a problem hiding this comment.
Nitpick: Fits on one line
| accessors: .accessors( | |
| AccessorDeclListSyntax([getAccessor]) | |
| ), | |
| accessors: .accessors(AccessorDeclListSyntax([getAccessor])), |
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Looks good to me. Thank you 👍🏽
|
@swift-ci Please test |
|
Could you run swift-format on your changes as described in CONTRIBUTING.md? |
Head branch was pushed to by a user without write access
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
2 participants
Fixes async/throws being dropped when converting zero-parameter functions to computed properties. Fixes #3235
Tested locally. Please let me know if you’d like me to make any changes or add test cases.