Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update Editors/README.md #1184

Merged
merged 8 commits into from
May 15, 2024
Merged

Update Editors/README.md #1184

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d826569
Update README.md
yaroslavyaroslav Apr 18, 2024
18b7c32
Review fixes
yaroslavyaroslav Apr 18, 2024
63fe6c1
Update README.md
yaroslavyaroslav Apr 20, 2024
4d06e65
Update README.md
yaroslavyaroslav Apr 21, 2024
87f9aa4
Update README.md
yaroslavyaroslav Apr 21, 2024
19d1893
Update README.md
yaroslavyaroslav Apr 21, 2024
8e7df19
Update README.md
yaroslavyaroslav Apr 23, 2024
491103d
Update README.md
yaroslavyaroslav Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 11 additions & 35 deletions Editors/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link

@rchl rchl Apr 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The package is called LSP-SourceKit and not SourceKit-LSP (referring to open 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 default xcrun (I'm not familiar with xcrun).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Fair point, fixed.
  2. I'd rather leave this one to @ahoppen decision.
  3. This one is interesting. AFAIK 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 from xcrun sourcekit-lsp up to sourcekit-lsp could be a better choice. Anyway, it was discussed just right here, and Alex insisted to keep this line in anyway.

Copy link

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 than xcrun sourcekit-lsp then this should be updated in the LSP-SourceKit package and then this override could be removed here.

Copy link
Contributor Author

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

Copy link
Member

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 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?

Copy link

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?

Copy link
Member

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.

Copy link
Contributor Author

@yaroslavyaroslav yaroslavyaroslav Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@rchl rchl Apr 23, 2024 •
edited
Loading

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 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:

  1. Update selector in LSP-SourceKit to include all relevant file types.
  2. Disable LSP-SourceKit server by default (have enabled: false in its default settings)
  3. 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.

Copy link
Member

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. 🙏🏽


```json
{
"clients":
{
"SourceKit-LSP":
{
"enabled": true,
"command": [
"command": [
"<sourcekit-lsp command>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@yaroslavyaroslav yaroslavyaroslav Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the right course of action would be to:

  • Change LSP-SourceKit to run sourcekit-lsp instead of xcrun sourcekit-lsp. /usr/bin/sourcekit-lsp is just a shim around xcrun sourcekit-lsp.
  • Then we don’t even need to include the command here.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 😀

Copy link

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So again, let's sum things up:

  1. The scope could be declared only once in a settings config, so it could be either all or nothing.
  2. If we declare the full scope of the server, we should disable it by default, due to potential conflicts with clang one.
  3. 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 c scope 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"
},
]
}
}
]
}
```

Expand Down