-
Notifications
You must be signed in to change notification settings - Fork 276
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
Update Editors/README.md #1184
Update Editors/README.md #1184
Changes from 1 commit
d826569
18b7c32
63fe6c1
4d06e65
87f9aa4
19d1893
8e7df19
491103d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,56 +22,32 @@ The [Swift for Visual Studio Code extension](https://marketplace.visualstudio.co | |
|
||
## Sublime Text | ||
|
||
Before using SourceKit-LSP with Sublime Text, you will need to install the LSP package from Package Control. To configure SourceKit-LSP, open the LSP package's settings. The following snippet should be enough to get started with Swift. | ||
Before using SourceKit-LSP with Sublime Text, you will need to install the LSP package from Package Control. And you're ready to go if you have `xcrun` in the `$PATH`. | ||
|
||
You will need the path to the `sourcekit-lsp` executable for the "command" section. | ||
To configure SourceKit-LSP additionally, open the SourceKit-LSP package's settings by typing in command palette `Preferences: LSP-SourceKit Settings`. The following snippet should be enough to get started with Swift and Objective-C/C++ and the custom path to `sourcekit-lsp` executable. | ||
|
||
```json | ||
{ | ||
"clients": | ||
{ | ||
"SourceKit-LSP": | ||
{ | ||
"enabled": true, | ||
"command": [ | ||
"command": [ | ||
"<sourcekit-lsp command>" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated, but isn't this man should be cross-platform? I mean the default client side command is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding is that if you install Xcode, you get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have nothing to complain here. I'm fine with either way. Just mentioning, that this block could be skipped completely if the minimalism matters. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the right course of action would be to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, my understanding was that we enabled it by default for Swift but disabled it by default for all the clang languages. But I assume that’s not possible? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nah, I'd rather leave this to answer to @rchl he definitely better in this 😀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I meant before is what @yaroslavyaroslav said and did. To have it enabled by default for Swift and for the user to be able to opt-in into other file types, could be done but it would be less user friendly. The user would have to go into LSP-SourceKit settings and override the So not sure what's better. I would have to leave the decision to you two. I don't even code in Swift. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So again, let's sum things up:
So after all, I believe that this is the best option that we got, to make it less error prone on one hand and to try to keep UX at its best on the other. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ahoppen FYI: the PR sublimelsp/LSP-SourceKit#7 is merged, so it's safe to merge this one now as well. |
||
], | ||
"env": { | ||
// To override the toolchain, uncomment the following: | ||
// "SOURCEKIT_TOOLCHAIN_PATH": "<path to toolchain>", | ||
}, | ||
"languages": [ | ||
], | ||
"languages": [ | ||
{ | ||
"scopes": ["source.swift"], | ||
"syntaxes": [ | ||
"Packages/Swift/Syntaxes/Swift.tmLanguage", | ||
"Packages/Decent Swift Syntax/Swift.sublime-syntax", | ||
], | ||
ahoppen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"languageId": "swift" | ||
"languageId": "c" | ||
}, | ||
{ | ||
"scopes": ["source.c"], | ||
"syntaxes": ["Packages/C++/C.sublime-syntax"], | ||
"languageId": "c" | ||
"languageId": "c++" | ||
}, | ||
{ | ||
"scopes": ["source.c++"], | ||
"syntaxes": ["Packages/C++/C++.sublime-syntax"], | ||
"languageId": "cpp" | ||
"languageId": "objc" | ||
}, | ||
{ | ||
"scopes": ["source.objc"], | ||
"syntaxes": ["Packages/Objective-C/Objective-C.sublime-syntax"], | ||
"languageId": "objective-c" | ||
"languageId": "objc++" | ||
}, | ||
{ | ||
"scopes": ["source.objc++"], | ||
"syntaxes": ["Packages/Objective-C/Objective-C++.sublime-syntax"], | ||
"languageId": "objective-cpp" | ||
"languageId": "swift" | ||
}, | ||
] | ||
} | ||
} | ||
] | ||
} | ||
``` | ||
|
||
|
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.
LSP-SourceKit
and notSourceKit-LSP
(referring toopen the SourceKit-LSP package's settings
).the following snippet...
text is misplaced since there is no snippet directly below.custom path to...
refers to? It doesn't seem like the code snippet below refers to a path but just to a binary name. And why would it be needed instead of using the defaultxcrun
(I'm not familiar withxcrun
).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.
xcrun
is a part of xcodebuild tools only, thus it provided on macOS only, so in this case, since ST is focused on being crossplatfrom, maybe to change the default setup fromxcrun sourcekit-lsp
up tosourcekit-lsp
could be a better choice. Anyway, it was discussed just right here, and Alex insisted to keep this line in anyway.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.
If
sourcekit-lsp
is a better command thanxcrun sourcekit-lsp
then this should be updated in theLSP-SourceKit
package and then this override could be removed here.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.
Again it's on Alex duty to decide
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 agree, it would be better to update that package. Also, do we even need to specify the
selector
here or could that also be sunk down into theLSP-SourceKit
package, in which case no additional configuration would be necessary?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 LSP-SourceKit package by default only enables server for swift files. Does it make sense to enable it for c/c++/objc by default? I guess it could be problematic to have it running in projects that are not really swift projects?
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, I see. The VS Code extension specifies support for C/C++/ObjC/ObjC++ by default, so I think that is a default that we can use here as well. That way, they are aligned.
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's some kind of similar to suggesting a feature to macOS team by referencing to Windows design decision 😀 It could work somewhat with zed though.
Anyway I'd rather agree with that, there's a loosing chance that some one who working on plain c/c++ codebase would occasionally install and turn on globally LSP-SourceKit that'll brings to a lsp servers conflict on an ST side.
So if @rchl has nothing against that, I can provide another LSP-SourceKit that (a) extends its scope to the list above and (b) replaces a given
xcrun sourcekit-lsp
command to/usr/bin/sourcekit-lsp
or whatever that we would decide in the thread above.I've made a draft PR to a plugin repo with both changes at once: selectors scope and executable path. So the rest is to conclude smth about both of those.
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 think that replacing
xcrun sourcekit-lsp
withsourcekit-lsp
should be done in the LSP-SourceKit package. At least as long as it's tested on mac.As for enabling for other file types... I suppose it could be a double edged sword. On one hand it would potentially enable server in projects that don't use swift. On the other hand, not enabling for other file types probably makes the server less effective in swift projects.
So one way of handling that could be to:
enabled: false
in its default settings)So users would explicitly opt-in to it per-project.
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.
That sounds like a great idea to me.
Also @rchl: Thanks for engaging in this discussion and sharing your knowledge of ST packages. That’s really valuable. 🙏🏽