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 3 commits
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 14 additions & 36 deletions Editors/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,56 +22,34 @@ 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`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would change this to sourcekit-lsp instead of xcrun because we don’t actually invoke xcrun.

Suggested change
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.


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

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
Collaborator

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
Collaborator

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

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

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
Collaborator

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


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.
Copy link

@rchl rchl Apr 20, 2024 •
edited

Choose a reason for hiding this comment

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

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.

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

Copy link

@rchl rchl Apr 21, 2024 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


```json
{
"clients":
{
"SourceKit-LSP":
{
"enabled": true,
"command": [
"<sourcekit-lsp command>"
],
"env": {
// To override the toolchain, uncomment the following:
// "SOURCEKIT_TOOLCHAIN_PATH": "<path to toolchain>",
},
"languages": [
"command": [
"sourcekit-lsp"
],
"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