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.

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

add "--lsp" flag to bsc to start an LSP server #668

Merged
merged 3 commits into from
Aug 18, 2022
Merged

add "--lsp" flag to bsc to start an LSP server #668

merged 3 commits into from
Aug 18, 2022

Conversation

ZeeD
Copy link
Contributor

@ZeeD ZeeD commented Aug 17, 2022

I'm not sure this is the best approach. Please suggest a better integration - my real doubt is that almost no flow is shared between the LSP and the °normal° bsc functionality

@ZeeD ZeeD mentioned this pull request Aug 17, 2022
@TwitchBronBron
Copy link
Member

TwitchBronBron commented Aug 17, 2022

Yeah, this is a bit more involved than I had intended. The primary reason for wanting to use something like --lsp was so we could keep the same entry point instead of exposing a whole separate binary name (like npx bsc-lsp). You can run programs without even installing them by using their full name. Like this: npx brighterscript --lsp it will download the package right then, and then spin it up and use it.

In the CLI, we have full control over how we run the code, so I was envisioning something more like this:

//cli.ts
let options = yargs
    // ...
   .option('lsp', { type: 'boolean', defaultDescription: 'false', description: 'Run brighterscript as a language server.' })
   .argv;

//if the lsp flag exists, ignore all other flags and run the language server instead
if (options.lsp === true) {
     const server = new LanguageServer();
     server.run();
} else{
    let builder = new ProgramBuilder();
    // ...

@ZeeD
Copy link
Contributor Author

ZeeD commented Aug 17, 2022

I changed the PR following your suggestions

@TwitchBronBron
Copy link
Member

Looks great! Did you have a chance to test this against your Eclipse implementation to verify that it works?

@ZeeD
Copy link
Contributor Author

ZeeD commented Aug 17, 2022

yes, it works as expected!

@TwitchBronBron TwitchBronBron merged commit a9e5e26 into rokucommunity:master Aug 18, 2022
@ZeeD ZeeD deleted the cli-lsp branch August 18, 2022 17:35
@TwitchBronBron
Copy link
Member

This change has been included in release v0.56.0

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