-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
WIP: Implement NeoVim Client #2098
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! First of all, thanks a lot for the hard work you've put into this. I'm not a maintainer of this repo, but I'm very interested in this, so would like to help you.
I've spent some time reviewing this today, but given the volume of your diff, I'm far from finished. I left a couple of comments and a few nitpicks for now.
Overall, I will be happy if this PR
- has more consistent typing (many functions are not typed at the moment)
- has some high-level description of the architecture
Code has a lot of moving parts, so it takes some time to understand how they all interact and communicate with each other. Both of these will help to understand the codebase quicker and make more meaningful contributions. Maybe if I had more context of how continue-dev or existing vscode plugin works, it would've been easier for me. But I don't, hence having some ad-hoc tips in this code will be very useful.
pattern = "*", | ||
callback = function() | ||
local current_file = vim.fn.expand('%:p') | ||
messenger.request("onDidChangeActiveTextEditor", { filepath = current_file }, function() end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused by this. It seems like there exist two types of messages
- pass through message type
- methods defined in the
ide_protocol
(for example, thisonDidChangeActiveTextEditor
)
The type definition for this function only mentiones those "pass through" types. This makes me wondering if this really needs to be handled by the same request
method? Or maybe it will be more clear to make two separate methods, request
and request_ide_protocol
? Or even call the ide_protocol
directly?
@@ -0,0 +1,243 @@ | |||
local M = {} | |||
|
|||
local log = require('continue.utils.log') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used anywhere
-- Function to get completion items | ||
function M.get_completion_items(prefix) | ||
local items = {} | ||
local candidates = prefix:sub(1, 1) == '@' and M.mentions or M.slash_commands |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even though after some struggle I understand that this either sets mentions or slash_commands to the candidates variable, it is very unclear on the first glance. It seems like candidates must be bool. I'd change it to something more obvious and add proper typing. For example:
-- Function to get completion items
--- @param prefix string
--- @return CompletionItem[]
function M.get_completion_items(prefix)
---@type CompletionItem[]
local items = {}
---@type CompletionItem[]
local candidates = {}
if prefix:sub(1, 1) == '@' then
candidates = M.mentions
elseif prefix:sub(1, 1) == '/' then
candidates = M.slash_commands
end
for _, item in ipairs(candidates) do
-- if vim.startswith(item.word, prefix) then
table.insert(items, {
word = vim.startswith(item.word, prefix) and item.word or (prefix .. item.word),
kind = item.kind,
menu = item.menu,
user_data = { has_submenu = item.has_submenu },
})
-- end
end
return items
end
And in general will be very useful to see better typing across this code, makes it much easier to understand.
for i, line in ipairs(lines) do | ||
for mention in line:gmatch('@%w+') do | ||
local start_col, end_col = line:find(mention, 1, true) | ||
if start_col then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if start_col then | |
if start_col and end_col then |
Otherwise the linter complains with Cannot assign integer|nil to parameter integer
vim.api.nvim_buf_set_option(bufnr, 'omnifunc', '') | ||
vim.api.nvim_buf_set_option(bufnr, 'completefunc', '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvim_buf_set_option
is deprecated as of nvim 0.10 https://neovim.io/doc/user/deprecated.html#nvim_buf_set_option()
vim.api.nvim_buf_set_option(bufnr, 'completefunc', '') | ||
|
||
-- Disable LSP completion | ||
for _, client in pairs(vim.lsp.get_active_clients({ bufnr = bufnr })) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deprecated as of nvim 0.11 https://neovim.io/doc/user/deprecated.html#vim.lsp.get_active_clients()
function ChatUI.new(stream_manager) | ||
local self = setmetatable({}, ChatUI) | ||
self.streamManager = stream_manager | ||
for name, stream in pairs(self.streamManager.streams) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name is not used
@qtnx this is quite a good start! I think given our status on being able to fully take over maintenance of another extension (we want to continue focusing our internal efforts on VS Code and Jetbrains for a time), and the early stage of development that this is in, the most helpful thing I can do is help shape guidelines around contribution and the direction of work that would make us most likely to adopt this sooner. My biggest worry is that our fast pace of development would cause this extension to need a lot of frequent changes when we either add new UI or modify how certain existing functionality works. By the time that we take on our next editor, we want the code to be in a spot that makes it feel comfortable to add 5 more. To get there, ideally:
And in general, keep an eye out for any situation where a) you find the core protocol to be awkward or missing something that you need (this is good feedback to help us solidify the API), or b) making a lot of logic/design decisions in the NeoVim client (maybe something we should discuss putting in the core or finding standard principles for, as we'll want such things for further IDEs) Anyway, I think you're doing great work. What I like about this NeoVim client is that because it doesn't use the React |
@sestinj First off, thank you, the team, and the community for creating a system that I believe is fantastic. I also agree that there’s currently a lack of documentation introducing how the core packages work overall. It would be incredibly helpful to have that, as @fspv mentioned above, not only to support plugin development for Neovim but also to help maintain other IDEs. I believe this could be easily achieved with some documentation generation tools, especially since the current code is already well-organized and logical. Personally, I’ve been able to understand how it all works by diving into each part without too much difficulty. Regarding breaking changes, they’re a natural part of any application. But I think they can be easily managed by tagging version releases and having a clear, coherent system for deprecating APIs and interfaces. I’ll aim to design this plugin to resemble VSCode as much as possible, so any future edits can be made more efficiently. Additionally, I’m considering separating this plugin into its own repository for better issue management and to receive more support from the Neovim community. I’d love to hear your thoughts on that. As for the development of this plugin, I’ve been quite busy with work lately, so I haven’t had much time to continue working on it. However, I plan to set aside some time this weekend to finish a couple of the TODO items. @fspv, thanks for taking the time to review. As Sestinj mentioned, there isn’t much documentation available for Continue's technical core at the moment, so we’ll need to dive deep into the codebase to understand it. This plugin is still in its early stages, so there will likely be significant changes, and some parts are still in draft form. But I’ll focus on improving it in the upcoming updates. |
Definitely agreed we can improve the documentation! We'll aim to approach this with READMEs I think rather than polluting the general docs with highly development-focused content. We would like to keep Continue in a single mono-repo, for a few reasons, but one of them being that it creates a single center of activity for the community, and makes development setup easier. Probably the solution to triaging issues is to use tags, which can then be filtered. That said, until we are ready to take over full-time development/maintenance it will need to live in a fork. |
And yes, I think matching VSCode is a good plan. Looking at the code there's not too much that makes me worry about refactoring in the future |
Description
This PR creates a pure NeoVim plugin to connect to the Continue.dev core without changing the core.
This is a significant implementation, so all feedback and contributions are extremely welcome. If you'd like to contribute to this Pull Request, please follow the guidelines at:
https://github.com/qtnx/continue/blob/dev/extensions/neovim/README.md
Checklist
transformer.js
works (Currently not working due to missing binary import)ide_protocol.lua
works as expected (mostly AI gen)free-trial
APIScreenshots