Skip to content

Conversation

@yaroslavyaroslav
Copy link
Contributor

Spawned by discussions here and here.

  • Dropping macOS only xcrun referencing in the code.
  • Expanding selectors to support the whole range of languages that server supports.
  • Default path set to /usr/bin/sourcekit-lsp

- Expanding selectors to support the whole range of languages that server supports.
- Default path set to `/usr/bin/sourcekit-lsp`
@yaroslavyaroslav
Copy link
Contributor Author

Draft until the decision about the path and selectors list to be default will be made in this PR

@yaroslavyaroslav yaroslavyaroslav force-pushed the feature/configuration-update branch from 60e82d0 to 9fa986c Compare April 23, 2024 19:52
@yaroslavyaroslav yaroslavyaroslav marked this pull request as ready for review April 23, 2024 19:53
@yaroslavyaroslav yaroslavyaroslav force-pushed the feature/configuration-update branch from 9fa986c to bb6000d Compare April 23, 2024 19:57
@yaroslavyaroslav yaroslavyaroslav force-pushed the feature/configuration-update branch from bb6000d to aac861d Compare April 23, 2024 19:58
Co-authored-by: Rafał Chłodnicki <[email protected]>
@yaroslavyaroslav
Copy link
Contributor Author

Done

"languageId": "swift"
}
]
// Enable this server globally for all the scopes provided in "selector" property.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is not up to date since it we don't enable globally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I'm trying to say here, is the purpose of this setting, that toggles the server globally, so I believe that this is rather correct comment that describes what user will get if he/she overrides this setting.

Copy link
Member

Choose a reason for hiding this comment

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

The comment is for the enabled: false setting which doesn't make sense.
Also, even if it would correctly state the actual state, it would be stating the obvious so it's not needed.

Copy link
Contributor Author

@yaroslavyaroslav yaroslavyaroslav Apr 24, 2024

Choose a reason for hiding this comment

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

I disagree with that, an opaque setting "enabled": false in a random github repo is far for being self descriptive for a folk who is not familiar with both ST and LSP internals.

Folk that came here especially from an Apple repo most likely would have zero knowledge about this toggle purpose so we should guide him/her here without forcing he/she to dig the purpose of the config values deeper.

UPD: still rephrased this sentence a bit for a clarifying sake

Copy link
Member

Choose a reason for hiding this comment

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

Well, the main issue with that comment is that it's not stating the correct thing. If you want to keep it then make it something like:

Disabled globally. Run "LSP: Enable Language Server in Project" from the Command Palette to enable per-project.

The part about selector is stating the obvious so shouldn't need a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, the update of this issue was missed coz of force push and UPD of a previous comment.

I've updated the hint a bit, please consider it when you'll have time.

@yaroslavyaroslav yaroslavyaroslav force-pushed the feature/configuration-update branch from 8f6261d to 9019280 Compare April 24, 2024 10:44
@yaroslavyaroslav yaroslavyaroslav force-pushed the feature/configuration-update branch from 9019280 to e0d7559 Compare April 24, 2024 10:45
@rchl rchl merged commit 5ae783a into sublimelsp:master Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants