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

tool: LSP support #879

Draft
wants to merge 92 commits into
base: devel
Choose a base branch
from
Draft

tool: LSP support #879

wants to merge 92 commits into from

Conversation

bung87
Copy link
Contributor

@bung87 bung87 commented Sep 7, 2023

Summary

  • LSP support

Details


Notes for Reviewers

  • get some improvements idea in my mind but first release initial version

@haxscramper haxscramper added this to the Tooling milestone Sep 7, 2023
@saem saem added the tool Improvements to non-compiler tooling label Sep 8, 2023
@bung87 bung87 force-pushed the nimlsp5 branch 3 times, most recently from 0b95c1d to c82d0af Compare September 11, 2023 15:12
@saem
Copy link
Collaborator

saem commented Sep 11, 2023

can you drop the nimble and nims files, plus any unnecessary cfg files.

@bung87
Copy link
Contributor Author

bung87 commented Sep 11, 2023

can you drop the nimble and nims files, plus any unnecessary cfg files.

sure, as this will stay with main repository.

nimlsp/README.rst Outdated Show resolved Hide resolved
@saem
Copy link
Collaborator

saem commented Sep 11, 2023

it'll probably be easier for reviewing to avoid force push -- all the squashing is taken care of during merge.

@zerbina
Copy link
Collaborator

zerbina commented Sep 11, 2023

For the directory structure, I think nimlsp.nim should move besides nimsuggest.nim in the tools directory, with the nimlsppkg directory (which can then be renamed to just nimlsp) also moved to tools.

In addition, please:

  • move the outline command implementation changes into a separate PR
  • remove the nimsuggest.nim copy -- the changes necessary for the LSP should be merged into the existing module

@bung87
Copy link
Contributor Author

bung87 commented Sep 11, 2023

@zerbina
Copy link
Collaborator

zerbina commented Sep 11, 2023

Yep, the plan is to remove the custom nimsuggest protocol, and with that, nimsuggest itself.

Until that's done, nimsuggest has to keep working. Having a largely duplicate version of nimsuggest is a problem, since it means that potential fixes/changes need to be applied to both modules.

What I'm suggesting/requesting is the following:

  • nimsuggest.nim keeps implementing the nimsuggest server, as it does now
  • the module also exports the parts needed by the LSP
  • the LSP server (nimlsp.nim) imports the existing nimsuggest.nim
  • once the LSP server is able to fully replace nimsuggest.nim, the latter is merged into the LSP server's code or restructured
  • outline is part of nimsuggest lib, not belongs to old nimsuggest.nim

I'm not sure what you mean. The outlining implementation is not part of nimsuggest itself, yes, but the feature is still integrated into nimsuggest. In addition, moving to a parser-based implementation is independent from introducing LSP support, hence my request to split it into a separate PR.

Keeping PRs focused on a single area or feature helps with reviewing them, and writing commit messages is usually also easier when the PR is smaller.

@zerbina
Copy link
Collaborator

zerbina commented Sep 13, 2023

@bung87: Please stop force-pushing, thank you. As Saem said, it unnecessarily makes review harder.

In addition, if the PR is not finished, please turn it into a draft PR. Each push to a PR that is marked as "ready for review" will trigger a run of the CI, which adds unnecessary resource contention.

@bung87
Copy link
Contributor Author

bung87 commented Sep 13, 2023

guess I'll create another PR

Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

partial review

nimlsp/README.rst Outdated Show resolved Hide resolved
nimlsp/README.rst Outdated Show resolved Hide resolved
nimlsp/nimlsppkg/nimsuggest.nim Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tool Improvements to non-compiler tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants