-
-
Notifications
You must be signed in to change notification settings - Fork 247
feat: Add multiline input for repl #773
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
Conversation
|
The user can configure when followup prompt can be called using the followup_func_cb, then when the user sends an empty string the prompt is finished and sent to the server. dap.adapters.python = {
type = 'server';
host = '127.0.0.1';
port = 1337;
followup_func_cb = function(text)
if string.sub(text, -1) == ':' then
return true
elseif string.sub(text, -1) == '\\' then
return true
elseif select(2, string.gsub(text, '"""', "")) == 1 then
return true
end
return false
end
}The followup prompt change may be a breaking change, I know that in dap-cmp it always expects the prompt to be 'dap> '. |
lua/dap/repl.lua
Outdated
| local prompt_state = vim.fn.getbufvar(repl.buf, PROMPT_STATE) | ||
| if prompt_state == REGULAR_PROMPT and continue_prompt_fn(curr_text) then | ||
| vim.fn.setbufvar(repl.buf, PROMPT_STATE, FOLLOWUP_PROMPT) | ||
| vim.fn.setbufvar(repl.buf, 'curr_text', curr_text .. '\n') |
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.
Here, if you add the text as it comes, you will add the text with the \ character at the end. This probably works in python, but it doesn't work in other languages, like javascript. So, I think it would be nicer if the followup_func_cb would return the text to actually be processed, or nil if it considers that it's not a multi line prompt. Then we could be developing something like this:
followup_func_cb = function(text)
if string.sub(text, -1) == '\\' then
return string.gsub(text, "\\[ ]*$", "")
end
return nil
endfor javascript, or something like this, for python:
followup_func_cb = function(text)
if string.sub(text, -1) == ':' then
return text
elseif string.sub(text, -1) == '\\' then
return text
elseif select(2, string.gsub(text, '"""', "")) == 1 then
return text
end
return nil
endThis would give a much larger flexibility, since would basically work for any language, not just for python.
Of course, the repl.lua file should be modified a little:
diff --git a/lua/dap/repl.lua b/lua/dap/repl.lua
index ac8d21e..1d8a83c 100644
--- a/lua/dap/repl.lua
+++ b/lua/dap/repl.lua
@@ -227,20 +227,21 @@ local function handle_followup_input(curr_text)
return curr_text
end
local prompt_state = vim.fn.getbufvar(repl.buf, PROMPT_STATE)
- if prompt_state == REGULAR_PROMPT and continue_prompt_fn(curr_text) then
+ local txt = continue_prompt_fn(curr_text)
+ if prompt_state == REGULAR_PROMPT and txt ~= nil then
vim.fn.setbufvar(repl.buf, PROMPT_STATE, FOLLOWUP_PROMPT)
- vim.fn.setbufvar(repl.buf, 'curr_text', curr_text .. '\n')
+ vim.fn.setbufvar(repl.buf, 'curr_text', txt .. '\n')
vim.fn.prompt_setprompt(repl.buf, get_prompt())
return nil
elseif prompt_state == FOLLOWUP_PROMPT then
local previous_text = vim.fn.getbufvar(repl.buf, 'curr_text')
- if curr_text == '' then
+ if txt == nil then
curr_text = previous_text .. curr_text
vim.fn.setbufvar(repl.buf, PROMPT_STATE, REGULAR_PROMPT)
vim.fn.prompt_setprompt(repl.buf, get_prompt())
return curr_text
else
- vim.fn.setbufvar(repl.buf, 'curr_text', previous_text .. curr_text .. '\n')
+ vim.fn.setbufvar(repl.buf, 'curr_text', previous_text .. txt .. '\n')
return nil
end
endThere 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 am a bit hesitant to add such a feature since it wouldn't be obvious to the user what was the exact input sent to the debug adapter. But if the purpose of this logic is to only start multi-line mode, then we could add Shift-Enter binding that will enter multi-line mode and go to new line, this could be active without needing any additional configuration. Another possibility for the user is to use treesitter in followup_func_cb and if the line gives an error or in python's case is a class or function definition then return true.
@cosminadrianpopescu what do you think?
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 understand the concern. But the way is implemented right now, it only works for python.
then we could add Shift-Enter binding that will enter multi-line mode and go to new line, this could be active without needing any additional configuration
This sounds good, but it will only work in certain terminals. Shift + Enter is not very easy to set up in vim, due to terminal restrictions (tty is even more difficult).
Another possibility for the user is to use treesitter in followup_func_cb and if the line gives an error or in python's case is a class or function definition then return true.
This would not really help, since the text would still contain the \ character at the end, so still would not work in other languages.
Anoter possibility is to have a setting, which would indicate if the character should be added to the query or not. Like g:dap_repl_multiline_ignore. Something on those lines, if clarity is an issue.
However, in my oppinion, since the follow_func_cb is not mandatory, who will want to use it, will not have issues understanding the concept.
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.
Here is an example of how to use treesitter for this:
local string_queries = { '(ERROR) @capture', '(expression_statement (function) @capture)' }
local queries = {}
for _, query in ipairs(string_queries) do
table.insert(queries, vim.treesitter.query.parse_query('javascript', query))
end
local function followup_func_cb(text)
local parser = vim.treesitter.get_string_parser(text, 'javascript')
local tree = parser:parse()[1]
local results = {}
for _, query in ipairs(queries) do
for id, _, _ in query:iter_captures(tree:root(), text) do
table.insert(results, id)
end
end
if #results > 0 then
return true
end
return false
endHere are the results:

I think that in general the best way to check if a follow up prompt should appear is to use treesitter or with more complex logic rather than just looking at the last letter.
However, in my oppinion, since the follow_func_cb is not mandatory, who will want to use it, will not have issues understanding the concept.
I assume most people who use dap are not configuring it by themselves, either using a plugins that already provide the configuration or because they are using a neovim distribution.
If you are not convinced it's good enough let me know and I'll make the change.
d7fa2c8 to
cf84e46
Compare
|
@mfussenegger I made the requested changes. also I have not fully verified if the keybindings work since my terminal doesn't support it, but it did work when I set it to other bindings |
cf84e46 to
303cdf2
Compare
|
Gentle reminder. This would be a great feature for any python user. |
ede639a to
c95b7ae
Compare
|
man this would be awesome to have! <3 |
|
Also would absolutely love this feature! |
c95b7ae to
24d6a03
Compare
|
@mfussenegger This PR has been open for almost a year, Do you intend on merging this feature? Do you want me to make changes to the code? |
lua/dap.lua
Outdated
| ---@field options nil|AdapterOptions | ||
| ---@field enrich_config? fun(config: Configuration, on_config: fun(config: Configuration)) | ||
| ---@field reverse_request_handlers? table<string, fun(session: Session, request: dap.Request)> | ||
| ---@field is_multiline fun(text: string) |
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.
Can you add a documentation about is_multiline into doc/dap.txt, and provide some examples of it? Say how should one write this funtion for python or javascript adapter? Doesn't this function require some "context" (or some syntactic knowledge) to determine whether the input requires further lines or not?
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 found this #773 (comment), but this is not enough. There would be many counterexamples, and I don't think the sole argument text (the last line) is enough. It would need to take all the lines entered so far as well.
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.
Also the type annotation is not correct:
-is_multiline fun(text: string)
+is_multiline fun(text: string):booleanThere 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 guess you ask for support for something like this:
> function a() {
...
... }
in my current implementation it is not supported, I'll look into it soon...
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.
@wookayin I have looked at it again, and I think the current implementation is better.
The current implementation works like this: Any user input can be considered either a single line input, or an input that starts a multiline block. The function is_multiline differentiate between the two. Once you enter a multiline mode, you will stay there until a blank line is sent by the user, that is the reason why the context is not sent, because it's always the first line.
The alternative comes into place once you are inside a multiline mode, in that case for each input you query is_multiline to see if the user should continue to input lines.
I think that the first approach is much simpler and probably covers most cases needed, as I mentioned in the last comment, it doesn't cover cases in which you have blank lines inside your block, but I think there needs to be a good example to justify adding the feature.
I am mostly concerned about an invalid configuration of the is_multiline function that will make the user stuck in multiline mode, not able to exit, this is only a problem in the second implementation.
|
Hi people. Is this any near being implemented? Many thanks. |
24d6a03 to
f357e74
Compare
|
I have modified how is_multiline works according to @wookayin suggestion. Now this function is called for every input and multi line input. Here is an example of such a function for python (there is probably a better solution with treesitter): local function is_multiline(current_inputs)
local last_input = current_inputs[#current_inputs]
if #current_inputs > 1 then
if last_input == "" then
return false
end
return true
end
if string.sub(last_input, -1) == ':' then
return true
elseif string.sub(last_input, -1) == '\\' then
return true
elseif select(2, string.gsub(last_input, '"""', "")) == 1 then
return true
end
return false
endThe code should be also simpler now. In addition, I found that repl history is not working well with multi line prompts, I am not sure how to make it work so I disabled selecting any text that has a newline in history. |
Multiline support like this still feels a bit clunky to me as you can't edit previous lines and I don't see a way around that other than having some sort of first class support in vim/nvim. The Overall I'm still mixed on this - don't want to say no. Complexity wise it doesn't seem too bad, but I have some changes in mind for the REPL and I then don't want to get blocked by the multiline handling. |
|
Related: neovim/neovim#32420 |
|
Under the light of neovim/neovim#33371 or whatever ends up getting into core it's quite clear to me that native multiline is the way to go |
Resolves #665