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

Recent changes make it impossible to extend the API #7

Open
thegecko opened this issue Mar 28, 2024 · 4 comments
Open

Recent changes make it impossible to extend the API #7

thegecko opened this issue Mar 28, 2024 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@thegecko
Copy link

The exported API here: https://github.com/eclipse-cdt-cloud/vscode-clangd/blob/main/src/extension.ts#L18

is an instance of a single ClangdProjectService class. Exposing this as the root API means it is not possible to expose other features without adding them to the ClangdProjectService itself, creating a very ugly interface.

For example, this PR: clangd#575 is exposing access to the AST and wont play nicely with the changes in this fork.

Could you consider changing the exports to be more extensible, possibly employing the same versioned API in the PR mentioned above?

@thegecko thegecko added the bug Something isn't working label Mar 28, 2024
@tortmayr
Copy link

tortmayr commented May 3, 2024

Hi @thegecko

thanks for raising this. We are all in favor of adopting the exports to allow a more extensible API.
However, I would like to wait with this until clangd#575 is merged in the upstream project.

Then we can make the necessary changes in combination with an update to the latest upstream version.

@thegecko
Copy link
Author

thegecko commented May 3, 2024

Lol, I was asked to move that PR to come from this fork, hence finding the clash!

If the PR isn't accepted, I'm keen for it to live here :)

@tortmayr
Copy link

tortmayr commented May 3, 2024

Ah I wasn't aware that there already was an ongoing discussion about this.
In that case, we are happy to take this over and adapt your extension API to align it with the API mentioned in the PR.

@tortmayr tortmayr self-assigned this May 3, 2024
@thegecko
Copy link
Author

thegecko commented Jul 5, 2024

FYI, the API exposure PR has now been merged:

clangd#575

Can the additions in this repo be rebased accordingly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants