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

Support API usage when clangd is disabled or failed to initialize properly #728

Merged
merged 7 commits into from
Nov 18, 2024

Conversation

HighCommander4
Copy link
Contributor

Fixes #721

The code is reorganized slightly so that in the cases where the client
would be null, we don't create a ClangdContext object in the first place
@HighCommander4
Copy link
Contributor Author

@thegecko I can't actually add you as a reviewer on this patch, but would appreciate if you could have a look anyways, and see if it looks like a suitable alternative to #727

@HighCommander4
Copy link
Contributor Author

@thegecko I can't actually add you as a reviewer on this patch

(I guess that's because you're not a project member; and I don't have admin access to the repo to make you a project member. @hokein, perhaps you can help with that?)

Copy link
Contributor

@thegecko thegecko left a comment

Choose a reason for hiding this comment

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

👍 This looks a lot neater than #727, I've made one minor suggestion around a static factory function.

There are still a couple of non-null assertions which I addressed in #727. These could be picked up in a subsequent PR.

src/clangd-context.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@HighCommander4
Copy link
Contributor Author

Thanks!

There are still a couple of non-null assertions which I addressed in #727. These could be picked up in a subsequent PR.

* https://github.com/clangd/vscode-clangd/blob/c7c91e644eec4cd46b62770336fac2a268426675/src/install.ts#L123

* https://github.com/clangd/vscode-clangd/blob/c7c91e644eec4cd46b62770336fac2a268426675/src/clangd-context.ts#L123

I went ahead and fixed these.

@thegecko
Copy link
Contributor

Thanks for adding those changes, looks great.

@HighCommander4 HighCommander4 merged commit 4da3e1e into clangd:master Nov 18, 2024
1 check passed
HighCommander4 added a commit to HighCommander4/vscode-clangd that referenced this pull request Nov 19, 2024
 - The disposables passed in to install.activate() were not saved for
   cleanup
 - config.get() is not async
HighCommander4 added a commit to HighCommander4/vscode-clangd that referenced this pull request Nov 19, 2024
 - The disposables passed in to install.activate() were not saved for
   cleanup
 - config.get() is not async
HighCommander4 added a commit to HighCommander4/vscode-clangd that referenced this pull request Nov 20, 2024
 - The disposables passed in to install.activate() were not saved for
   cleanup
 - config.get() is not async
HighCommander4 added a commit that referenced this pull request Nov 20, 2024
Save the disposables passed in to install.activate() for cleanup
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.

The language client exposed by the plugin's API can be null
2 participants