Update Editors/README.md #1184
Update Editors/README.md #1184
Conversation
There was a problem hiding this comment.
Thanks for updating the instructions @yaroslavyaroslav. Two questions/comments inline
Editors/README.md
Outdated
| { | ||
| "enabled": true, | ||
| "command": [ | ||
| "command": [ | ||
| "<sourcekit-lsp command>" |
There was a problem hiding this comment.
I think we can just use sourcekit-lsp here, which should work if you have sourcekit-lsp in your PATH, which you should have if you have Xcode installed. We do the same for the vim instructions below.
There was a problem hiding this comment.
I've updated, but isn't this man should be cross-platform? I mean the default client side command is xcrun sourcekit-lsp that should just work if one has Xcode installed, therefore this setting is redundant at all. But I'm not sure about the picture on Linux and Windows though.
There was a problem hiding this comment.
My understanding is that if you install Xcode, you get /usr/bin/sourcekit-lsp (which is in your PATH), which is essentially a trampoline for xcrun sourcekit-lsp. So, if we use sourcekit-lsp as the command, it uses that on macOS. On Linux, we expect the toolchain to be in your path as well (which it usually is if you install it to /usr/bin, so then you also have sourcekit-lsp in your path). Given that this should work for most users, I think it’s good to require one less customization step from users.
There was a problem hiding this comment.
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.
I think the right course of action would be to:
- Change LSP-SourceKit to run
sourcekit-lspinstead ofxcrun sourcekit-lsp./usr/bin/sourcekit-lspis just a shim aroundxcrun sourcekit-lsp. - Then we don’t even need to include the
commandhere.
There was a problem hiding this comment.
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.
Nah, I'd rather leave this to answer to @rchl he definitely better in this 😀
There was a problem hiding this comment.
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 selector manually (we could leave commented-out, alternative selector to make it easier). So essentially what this guide said initially. But if the user does that manually, then he/she will still have the same problem that it might trigger in non-swift projects. Unless user overrides selector in project-specific settings (which is also possible) but again, less user friendly than just using Command Palette.
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.
So again, let's sum things up:
- The scope could be declared only once in a settings config, so it could be either all or nothing.
- If we declare the full scope of the server, we should disable it by default, due to potential conflicts with
clangone. - We can declare some limited scope as default (for instance objc/objc++/swift), but then it would take more effort from a user to expand it with c/c++ on his side rather than just send a single command in command palette. The other downside of this decision, is that all objc header files identifies as
cscope in ST by default, so it's likely would be a cause of an inconsistency issue in a UX with the sourcekit-lsp.
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.
@ahoppen FYI: the PR sublimelsp/LSP-SourceKit#7 is merged, so it's safe to merge this one now as well.
Swift-Next syntax highlight added
Editors/README.md
Outdated
|
|
||
| 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. |
There was a problem hiding this comment.
- The package is called
LSP-SourceKitand notSourceKit-LSP(referring toopen the SourceKit-LSP package's settings). - The
the following snippet...text is misplaced since there is no snippet directly below. - What does
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.
- Fair point, fixed.
- I'd rather leave this one to @ahoppen decision.
- This one is interesting. AFAIK
xcrunis 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-lspup tosourcekit-lspcould 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.
If sourcekit-lsp is a better command than xcrun sourcekit-lsp then this should be updated in the LSP-SourceKit package and then this override could be removed here.
There was a problem hiding this comment.
Again it's on Alex duty to decide
There was a problem hiding this comment.
If sourcekit-lsp is a better command than xcrun sourcekit-lsp then this should be updated in the LSP-SourceKit package and then this override could be removed here.
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 the LSP-SourceKit package, in which case no additional configuration would be necessary?
There was a problem hiding this comment.
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.
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.
The VS Code extension specifies support
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.
I think that replacing xcrun sourcekit-lsp with sourcekit-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:
- Update selector in LSP-SourceKit to include all relevant file types.
- Disable LSP-SourceKit server by default (have
enabled: falsein its default settings) - Add instructions in its readme that if the user wants to enable it, he/she has to run "LSP: Enable language server in project" from the command palette.
So users would explicitly opt-in to it per-project.
There was a problem hiding this comment.
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. 🙏🏽
Editors/README.md
Outdated
| 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. | ||
|
|
||
| To make your journey even more pleasant it's recomended to install [Swift-Next](https://github.com/Swift-Next/Swift-Next) package that provides advanced swift syntax highlighting support. |
There was a problem hiding this comment.
The LSP-SourceKit package recommends the Decent Swift Syntax. Why the discrepancy? Should the package's readme be updated?
But also it's not so much about being "more pleasent". A syntax is required for LSP to work. So I'm not sure what "more pleasent" refers to? Not having the syntax or to having some other syntax? In the latter case, I'm not sure if that's clear because nothing here mentions to install the other syntax either. If it's really better then you could update the LSP-SourceKit readme and then omit the whole paragraph here.
There was a problem hiding this comment.
I believe we have this talk before, when Swift-Next got included in Package Control initially. So after all folks decided to not replace any existing ST swift syntax package with this one, but to add tests and add it into the default packages list instead. In short the main reason was that this package is ST4 compatible only, which is not the case for the "default" Swift package provides ST3 and ST2 support as well. So here we are. The only thing I can say to this: someday I'll complete the tests task, but the day is not today.
The same reasoning is true when we're speaking about the replacement of Decent Swift Syntax with Swift-Next in LSP-SourceKit package readme. If it's acceptable to suggest the ST4 only option as the default option for this package — i'm up for it, and would love to update its readme as well. But if you prefer to keep the one that provides better backward capability, so there's nothing to do with LSP-SourceKit's readme.
UPD: As you've mentioned in the comment below:
but ST3 has not been relevant for many years now. ST4 is what matters.
I believe that this is yes to the question above, thus I'm going to update LSP-SourceKit readme with replacement of a syntax suggestion as well soonish.
There was a problem hiding this comment.
Yes, ST3 is not relevant anymore.
And also LSP-SourceKit is ST4-only so if "Swift-Next" is better, it should just recommend that.
But what I've mainly tried to say is that from the perspective of someone who just reads intructions here, there is not even a mention of installing any syntax so this is why I thought the wording here is not ideal. Many users will likely not read LSP-SourceKit readme separately if they are installing it based on those instructions.
There was a problem hiding this comment.
Yeah, I see your point, but personally I think that it's unlikely that a user would bother to install an lsp plugin for language while intentionally skipping its syntax highlighting part. So this is why I chose this form rather than more strict one.
There was a problem hiding this comment.
I agree that we should make the suggestion for the syntax highlighting package consistent between the documentation of LSP-SourceKit and this REAMDE. I take it that there is consensus that Swift Next is superior to Decent Swift Syntax (especially because there is a plan to get Swift Next into the default packages) except for the part that Swift Next is not ST4-compatible. But if LSP-SourceKit isn’t either, I don’t think that’s a blocker. Could we
- Change LSP-SourceKit to also refer to Swift Next?
- I agree that we should rephrase this sentence to be a little less optional, on the lines of: Also install Swift Next to use Swift in ST.
- I am not familiar with ST’s package infrastructure. But would it be possible to make LSP-SourceKit depend on Swift Next so users get the entire bundle when installing LSP-SourceKit?
There was a problem hiding this comment.
Change LSP-SourceKit to also refer to Swift Next?
It's updated now - sublimelsp/LSP-SourceKit#6
I am not familiar with ST’s package infrastructure. But would it be possible to make LSP-SourceKit depend on Swift Next so users get the entire bundle when installing LSP-SourceKit?
It's not technically possible right now for one package to depend on another package. There is a concept of dependencies but those are not packages and can't be installed on its own.
There was a problem hiding this comment.
It's updated now - sublimelsp/LSP-SourceKit#6
Thanks!
It's not technically possible right now for one package to depend on another package. There is a concept of dependencies but those are not packages and can't be installed on its own.
Ah, OK, I wasn’t aware of that. Let’s keep the suggestion in here then.
Editors/README.md
Outdated
| @@ -22,56 +22,18 @@ 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 and LSP-SourceKit packages from Package Control. And you're ready to go if you have `xcrun` in the `$PATH`. | |||
There was a problem hiding this comment.
I would change this to sourcekit-lsp instead of xcrun because we don’t actually invoke xcrun.
| Before using SourceKit-LSP with Sublime Text, you will need to install the LSP and LSP-SourceKit packages from Package Control. And you're ready to go if you have `xcrun` in the `$PATH`. | |
| Before using SourceKit-LSP with Sublime Text, you will need to install the LSP and LSP-SourceKit packages from Package Control. And you're ready to go if you have `sourcekit-lsp` in the `$PATH`. This is the case if you have Xcode installed on macOS or a [Swift Toolchain](https://www.swift.org/install/) installed on Linux. |
Editors/README.md
Outdated
|
|
||
| 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. |
There was a problem hiding this comment.
If sourcekit-lsp is a better command than xcrun sourcekit-lsp then this should be updated in the LSP-SourceKit package and then this override could be removed here.
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 the LSP-SourceKit package, in which case no additional configuration would be necessary?
| } | ||
| } | ||
| ``` | ||
| Before using SourceKit-LSP with Sublime Text, you will need to install the [LSP](https://packagecontrol.io/packages/LSP), [LSP-SourceKit](https://github.com/sublimelsp/LSP-SourceKit) and [Swift-Next](https://github.com/Swift-Next/Swift-Next) packages from Package Control. Then toggle the server on by typing in command palette `LSP: Enable Language Server Globally` or `LSP: Enable Language Server in Project`. |
There was a problem hiding this comment.
IIUC LSP-SourceKit is a small amount of configuration to enable sourcekit-lsp (which could be done in the LSP configuration)? And Swift-Next adds grammar files (vs the old https://github.com/quiqueg/Swift-Sublime-Package and https://github.com/colinta/decent-swift-syntax)?
If so, I think we should re-word this slightly to something like:
Before using SourceKit-LSP with Sublime Text, you will need to install the LSP from Package Control. From there you can either manually configure Swift and C-family languages to use
sourcekit-lsp, or use a community-provided package to set this up automatically (eg. LSP-SourceKit).
For syntax highlighting, how does this interact with the semantic highlighting provided by LSP? Is it merged after the response comes back? Something else? Depending on that we could also add eg.
Swift-Next (another community-provided package) can also be added for syntax highlighting prior to Sublime receiving a response from the LSP server.
EDIT: I just realized the above thread is all about this. Sounds like it isn't just for highlighting prior to the LSP response, but in any case, mentioning some summary of that would be useful here IMO.
Not for you in this PR, but we probably need to got through this doc and clear it up for the other editors in general 😅.
There was a problem hiding this comment.
Swift docs update
Ah sheesh, here we go again 😅
So let's break it down from top to bottom in one place
LSP-SourceKit package purpose
LSP-SourceKit is a small amount of configuration to enable
sourcekit-lsp(which could be done in the LSP configuration)?
It's no. It was so a while ago, but for at least a few years now it has been handled by a separate LSP-ServerName packages. As mentioned in a note on a link you provided in your suggested change snippet:
The external LSP-* helper packages already come with their setting file and a client configuration and you don't need to add anything to the global LSP settings. This section is only relevant if you want to add a new client configuration for a server that doesn't have a corresponding helper package.
So since we're having LSP-SourceKit helper package this is the main way to interact with a sourcekit-lsp server for ST4 user.
LSP-SourceKit maintaince
or use a community-provided package to set this up automatically (eg. LSP-SourceKit).
Technically speaking all the LSP stack for Sublime Text is community provided, including the main LSP itself. The main LSP package, along with the LSP-SourceKit helper, is stored in the same group on GitHub. Therefore, it makes sense to consider them both as either semi-official or community-driven simultaneously.
C family language support
either ... Swift and C-family languages to use
sourcekit-lsp
This is true, the default configuration of LSP-SourceKit package is set to support all c, c++, objc, objc++ and swift. And at the same time this is the reason why the plugin installs in it's disabled state. We had long discussion about how to make it best and conclude that this is the best scenario from UX perspective.
To elaborate, most ST users unaware of sourcekit-lsp capabilities to support c family languages, and they can use clang for that purpose in their swift-unrelated projects. So, this bit says no to either include c, c++ into LSP-SourceKit default scope or toggle it on by default.
From the other perspective, if to exclude those scopes from the default setup, ST barely could differentiate c/c++ header file from objc one, which leads to it will not call this server for objc headers if they are appeared in project without additional user's effort to setup scope properly. And this is requires more effort that just run a single command from within command palette to explicitly toggle this server for a project or globally.
This is how we went here. To include the full list of scopes that sourcekit-lsp supports in the default setup, but to toggle them off on installation, to not break other plain c/cpp projects.
Miscellaneous Syntax highlighting questions
Various ST swift syntax packages
And Swift-Next adds grammar files (vs the old https://github.com/quiqueg/Swift-Sublime-Package and https://github.com/colinta/decent-swift-syntax)?
That's right. ST suffered from a very limited swift syntax highlight support until Swift-Next release, which is finally quite mature in just this. Now this package is on its (long) way to being included in predefined packages list
As my college claimed above the one grammar file is strictly required to make LSP helper package work. So me and ST team are suggesting this one among the others.
Semantic syntax highlight support
For syntax highlighting, how does this interact with the semantic highlighting provided by LSP? Is it merged after the response comes back?
Yep, grammar loads first, almost instantly on a file open, then, it could be expanded with semantic syntax tokens provided by a server. Now this feature disabled by default (to the LSP package in general). I've tried to enable this for swift sources and have to say it's pretty useless for ST for now, as this kind of syntax highlight just make things worst comparing to a grammar one. Didn't dig deep enough yet, but I believe the cause is that there is token naming mismatch between those that server sending and ST are working with.
EDIT: Various English grammar fixes
There was a problem hiding this comment.
Didn't dig deep enough yet, but I believe the cause is that there is token naming mismatch between those that server sending and ST are working with.
Ah, I wonder if this was while we had an extra kind inbetween the built-in ones. Technically this is allowed by the spec, but most clients seem to ignore the mapping 😅. That was changed in #1012.
There was a problem hiding this comment.
Heh, yeah, it was definitely after that PR, like a few weeks ago. It's because before that LSP package had a bug that broke swift semantics syntax highlight completely 😅.
Now it's fixed, but yet if to enable it's literally colors almost all the tokens in the same color for a vast majority of ST color schemes that I've tried.
I saw an option to remap tokens names right within LSP helper scope, so I think it could be a solution to that.
There was a problem hiding this comment.
Thank you for the detailed summary @yaroslavyaroslav, that was super helpful!
| } | ||
| } | ||
| ``` | ||
| Before using SourceKit-LSP with Sublime Text, you will need to install the [LSP](https://packagecontrol.io/packages/LSP), [LSP-SourceKit](https://github.com/sublimelsp/LSP-SourceKit) and [Swift-Next](https://github.com/Swift-Next/Swift-Next) packages from Package Control. Then toggle the server on by typing in command palette `LSP: Enable Language Server Globally` or `LSP: Enable Language Server in Project`. |
There was a problem hiding this comment.
Didn't dig deep enough yet, but I believe the cause is that there is token naming mismatch between those that server sending and ST are working with.
Ah, I wonder if this was while we had an extra kind inbetween the built-in ones. Technically this is allowed by the spec, but most clients seem to ignore the mapping 😅. That was changed in #1012.
|
@swift-ci please test |
|
@yaroslavyaroslav It looks like you deleted your repository and thus we can’t merge this PR. Would you mind setting it up again? Otherwise, I can also re-open the PR in your name. |
|
@ahoppen oh my bad, I still have in on my laptop will recreate PR today. |
|
Must be fixed now. Sorry about the mess that I brought, was today's old when knew that GH uses a single instance on a user side for all forks within a repo. |
|
Thanks for re-opening the PR. |
|
@swift-ci Please test macOS |
In Sublime Text setup part, to reflect recent LSP package API changes.