-
Notifications
You must be signed in to change notification settings - Fork 304
Support cross-language rename between Swift and clang languages #1038
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
Conversation
@swift-ci Please test |
530443b
to
7a39019
Compare
@swift-ci Please test |
@swift-ci Please test Windows |
case malformedSwiftToClangTranslateNameResponse(SKDResponseDictionary) | ||
case malformedClangToSwiftTranslateNameResponse(SKDResponseDictionary) |
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.
Is the response useful other than for the description? If not, maybe just pass in the description directly?
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 thought that we pass in the SKDResponseDictionary
because that gives us more type safety about what the associated type is than when it’s String
. But I don’t have strong opinions either way.
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'd prefer String
, only so that we don't keep the SKDResponseDictionary
around. But 🤷 doesn't really matter either way.
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.
It won’t stay around for long anyway, let’s just keep it.
Sources/SourceKitLSP/Rename.swift
Outdated
/// All properties refer to the definition symbol. For example, an Objective-C method named `performAction:with:` that | ||
/// is imported to Swift as `perform(action:with:)` will always have its definition in an Objective-C source file and | ||
/// the definition name will always be `performAction:with:`, even if rename is invoked from Swift. | ||
public actor TranslatableName { |
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 do wonder if this is worth it, or if we should just do a quick iteration over the index results and pick a location for the swiftName case up front. Lazy is nice and all, but I'm not sure it matters that much here? Seems like it would avoid a lot of complexity.
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 changed the lazy name translation in TranslatableName
to an up-front translation in 0336bf4. What do you think?
@swift-ci Please test |
@swift-ci Please test Windows |
We didn’t have any test coverage for Objective-C packages. Let’s add some.
This adds support for name translation between Swift anc clang languages to allow renaming across those language boundaries. Rename is always initiated at the symbol’s definition, so when renaming an Objective-C symbol from Swift, the user is prompeted to enter the new Objective-C method name. rdar://118996461
6a539ff
to
7748f37
Compare
@swift-ci Please test |
…ation This simplifies the code.
7748f37
to
b9ccf57
Compare
@swift-ci Please test |
@swift-ci Please test Windows |
@swift-ci Please test Windows |
This adds support for name translation between Swift anc clang languages to allow renaming across those language boundaries.
Rename is always initiated at the symbol’s definition, so when renaming an Objective-C symbol from Swift, the user is prompeted to enter the new Objective-C method name.
rdar://118996461