-
Notifications
You must be signed in to change notification settings - Fork 332
Add documentation for refactoring actions #2414
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR adds comprehensive documentation for SourceKit-LSP's refactoring capabilities, addressing issue #1613. The documentation explains how to invoke refactoring actions and catalogs all available refactorings.
Key changes:
- Created new "Refactoring Actions.md" documentation file with detailed coverage of semantic, syntactic, and package manifest refactorings
- Added reference to the new documentation in the main Documentation README
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Documentation/Refactoring Actions.md | New comprehensive guide covering refactoring action types, invocation methods, and editor support |
| Documentation/README.md | Added link to new Refactoring Actions documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ahoppen
left a comment
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 a lot. I think this is the best location for the list of refactoring actions for now. Now we just need to remember to keep it up-to-date 😉
Out of curiosity: How did you compose this list? Did you scrape through the different code bases to find them or did you try different locations in source files and checked which refactoring actions you found?
Oh thanks lol , I Found a master list of syntactic refactoringss in |
Co-authored-by: Alex Hoppen <[email protected]>
Documentation/Refactoring Actions.md
Outdated
|
|
||
| These refactorings require full semantic analysis of the code and are provided by the Swift compiler's sourcekitd. | ||
|
|
||
| #### Cursor-Based Refactorings |
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.
Hmm, actually just thought about this again and I don’t think it makes too much sense to organize these by where the refactoring actions are implemented because users likely don’t care about that. I think it would make more sense to group them by what kind of code they apply to, similar to how the package manifest refactoring actions are in one group. Maybe we could have one section for literals, one for functions, one for types etc. I didn’t look through all entries, so there might be a better grouping than what I’ve suggested, I’ll leave that to you 😉
Also, I really like the trigger column for the syntactic refactoring actions. If you could add that to all refactoring actions, I think that would be amazing. Maybe also verify that what’s written there is actually correct 😊
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.
oh okaey on it currently working on the test case , looks a bit tricky , ill come baxk to this after a while 🤗
e10e339 to
222cd51
Compare
|
is this better ? , i did go through most of them myself |
|
hmm seems like some of them dont work this might take a while :/ |
|
i tried writing individual small lines of code to test all these but some of them seems to have failed maybe i overlooked something |
ahoppen
left a comment
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.
Very nice survey of all the refactorings and how to invoke them. I now want to see somebody say that Swift didn’t have any refactoring actions (I personally wasn’t aware we had this many, great to see them in one location). I put some clarification suggestions inline. In terms of the general document structure, I’d order it from the smallest code unit (literal) to the biggest (the package) as follows:
- Strings and Literals
- Let’s inlcude the String Concatenation section in here
- Control Flow
- Macros
- Functions and Closures
- Async/Await
- Types and Protocols
- Source Organization
- Package.swift Manifest Editing
yes while individually testing some i noticed some of them werent working as intended but majority of them worked some of those were i think macros , i dont have more details right now as im afk here is a snippet i had saved in my phone for looking it up on the go 😅! macro there is still some more i have to go through i think |
I think all of them worked for me as I expected. If you anything that didn’t work, please file issues for those 🙏🏽 |
05da106 to
b60119a
Compare
ahoppen
left a comment
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.
Looks good. Thanks a lot for putting this document together 🙏🏽
|
@swift-ci Please test |
|
out of curiousity : is it neccessary to test the docs with ci ? do they break builds , dosent it cost more ( idk how this works jsut curious ) |
|
We have a requirement that all PRs need to be tested with CI and there’s no exception rule for documentation. Overall, I think it’s less work to run CI on these PRs than trying to maintain exception lists. |
|
@swift-ci Please test Windows |
addresses #1613
im not sure if they are the best docs and i did have a bit of ai help to go through most of the stuff to ensure i didnt miss any , pls do correct me if i missed anything