-
Notifications
You must be signed in to change notification settings - Fork 425
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
Code action to convert between computed property and stored property #2712
Code action to convert between computed property and stored property #2712
Conversation
defcaca
to
7ee1cf2
Compare
Thanks for review, @ahoppen! |
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.
Thanks for the adjustments. I have a few comments, mostly regarding trivia handling. The overall shape looks good. It would also be good to add test cases for the examples that I mentioned.
7ee1cf2
to
107417a
Compare
Need help with the Test: let baseline: DeclSyntax = """
var defaultColor: Color {
return Color()
/* some text */
}
"""
let expected: DeclSyntax = """
let defaultColor: Color = Color()
/* some text */
""" Test: let baseline: DeclSyntax = """
var defaultColor: Color {
return Color() /* some text */
}
"""
let expected: DeclSyntax = """
let defaultColor: Color = Color() /* some text */
""" |
How about adding the trivia of the dropped braces to the initializer? Ie something like initializer.leadingTrivia = accessorBlock.leftBrace.leadingTrivia + accessorBlock.leftBrace.trailing + initializer.leadingTrivia
initializer.trailingTrivia += accessorBlock.rightBrace.leadingTrivia + accessorBlock.rightBrace.trailing |
107417a
to
d5b3c15
Compare
@ahoppen, done |
eac08a4
to
ebf4299
Compare
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.
The results are looking great!
098e9b3
to
33d4cee
Compare
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.
Looking great. Let’s get this merged.
@swift-ci Please test |
33d4cee
to
c10ac8a
Compare
@swift-ci please test |
@swift-ci please test windows |
@swift-ci Please test Linux |
Resolves swiftlang/sourcekit-lsp#1245