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

LSP for CodeCompass #599

Merged
merged 28 commits into from
Feb 4, 2024
Merged

LSP for CodeCompass #599

merged 28 commits into from
Feb 4, 2024

Conversation

Kalfou
Copy link
Collaborator

@Kalfou Kalfou commented May 16, 2023

LSP handler and C++ LSP plugin added with some utility functions and a common interface for LSP plugins.

Enhances #335.

@mcserep mcserep added Kind: Enhancement 🌟 Target: LSP Issues related to Language Server Protocol (LSP)-based operation, as opposed to the Web app. labels May 17, 2023
@mcserep mcserep added this to the Release Gershwin milestone May 17, 2023
Copy link
Collaborator

@intjftw intjftw left a comment

Choose a reason for hiding this comment

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

I haven't checked the entire cpplspservice.cpp class, but I looked through all the other ones. I'm starting the review so @Kalfou can start adding fixes.

plugins/cpp/service/include/service/cppservice.h Outdated Show resolved Hide resolved
plugins/cpp/service/src/diagram.cpp Outdated Show resolved Hide resolved
webserver/include/webserver/thrifthandler.h Outdated Show resolved Hide resolved
webserver/include/webserver/pluginhelper.h Show resolved Hide resolved
webserver/include/webserver/lsphandler.h Outdated Show resolved Hide resolved
service/lsp/CMakeLists.txt Show resolved Hide resolved
service/lsp/include/lspservice/lsp_types.h Outdated Show resolved Hide resolved
webserver/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@intjftw intjftw left a comment

Choose a reason for hiding this comment

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

I found the code generally nice and clean, and architecturally correct. I have two more small change requests.

plugins/cpp_lsp/service/src/cpplspservice.cpp Outdated Show resolved Hide resolved
plugins/cpp_lsp/service/src/cpplspservice.cpp Outdated Show resolved Hide resolved
@intjftw
Copy link
Collaborator

intjftw commented May 27, 2023

@Kalfou please write a short description of the LSP options in the README, and build and install directions in the README of the plugin.

Considering the plugin, I only have one issue: currently, the package.json file contains default values for CodeCompass properties (project path, name, webserver port number etc.). This solution is quite inflexible, and increases the chance of user errors. In my opinion, the plugin should be able to handle several projects. Also, the json file or the README should be complemented with a template on how to add new project properties to the file.

@mcserep
Copy link
Collaborator

mcserep commented Jun 9, 2023

@Kalfou I have fixed and refactored the outdated CI in #596, please rebase this PR onto master (or merge it), so the pipeline can be executed properly.

@Kalfou
Copy link
Collaborator Author

Kalfou commented Sep 25, 2023

Hi,
Is there anything that still needs to be done to make this merge ready?

@Kalfou Kalfou requested a review from intjftw October 5, 2023 08:55
Copy link
Collaborator

@mcserep mcserep left a comment

Choose a reason for hiding this comment

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

Sorry for the late response @Kalfou and thanks for updating the PR according to the reviews. I think this is ready for merge 🚀

It would be also nice to publish the VS Code LSP plugin somewhere, as currently it is only available in private repository on the ELTE FI Gitlab server.
I think it should either be added to this repo (in a separate PR) or maybe a new repo could be created for LSP plugins (with the possible future work towards other IDEs).

@mcserep mcserep merged commit d761423 into Ericsson:master Feb 4, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Kind: Enhancement 🌟 Target: LSP Issues related to Language Server Protocol (LSP)-based operation, as opposed to the Web app.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants