Skip to content

feat: watch consumer#229

Merged
rcarriga merged 10 commits into
masterfrom
feat/watch-consumer
Jun 15, 2023
Merged

feat: watch consumer#229
rcarriga merged 10 commits into
masterfrom
feat/watch-consumer

Conversation

@rcarriga
Copy link
Copy Markdown
Collaborator

@rcarriga rcarriga commented Apr 2, 2023

No description provided.

@rcarriga rcarriga force-pushed the feat/watch-consumer branch from 33bd89e to 8495582 Compare April 2, 2023 17:06
@scottming
Copy link
Copy Markdown
Contributor

scottming commented Apr 4, 2023

For this feature, I am very excited. May I ask if it is necessary to have LSP support for these three functions:

selflsp_client.request.textDocument_documentSymbol
lsp_client.request.textDocument_prepareCallHierarchy
lsp_client.request.callHierarchy_outgoingCalls

If one of the LSPs does not support it, then I won't be able to use the watch feature, right?

@rcarriga
Copy link
Copy Markdown
Collaborator Author

rcarriga commented Apr 7, 2023

This is still very much WIP, I'm playing around with several approaches so users will have options 😁

@rcarriga rcarriga force-pushed the feat/watch-consumer branch 3 times, most recently from 59a5669 to 92badcc Compare April 10, 2023 10:13
@rcarriga
Copy link
Copy Markdown
Collaborator Author

OK I've switched the implementation to use a mix of treesitter (which nearly all adapters are already using) and the
textDocument/defintion LSP request which I believe all servers support.

This PR is now pretty stable so it'd be great if people wanted to test and provide any feedback before merging. One
thing to note though is that this change is behind a breaking change in #228 which some adapters will need to update to.
The only ones that I know that must change for regular use (many more will have their unit tests break) are neotest-jest
and neotest-dotnet.

@scottming
Copy link
Copy Markdown
Contributor

Thank you for your great work. I like this new approach of tree-sitter + definition. I just implemented the definition feature in Lexical (another new LSP for Elixir), so I am happy to test this feature in the coming days.

@scottming
Copy link
Copy Markdown
Contributor

scottming commented Apr 11, 2023

I found some bugs and have fixed some of them in this PR #232 , including issues with symbol range and definition format.

However, I still cannot use the watch feature. When there are multiple linked files, I am unable to see this message:

WARN | 2023-04-11T17:36:26Z+0800 | ...ing/Code/neotest/lua/neotest/consumers/watch/watcher.lua:83 | Found 1 linked files /Users/scottming/Code/python_demo/tests/test_demo.py for /Users/scottming/Code/python_demo/tests/test_demo.py
WARN | 2023-04-11T17:36:26Z+0800 | ...ing/Code/neotest/lua/neotest/consumers/watch/watcher.lua:162 | Built dependencies in 9 ms for /Users/scottming/Code/python_demo/tests/test_demo.py::test_foo : {
  ["/Users/scottming/Code/python_demo/tests/test_demo.py"] = { "/Users/scottming/Code/python_demo/tests/test_demo.py" }
}

So the first bug I have no idea is can't start the watcher when there are multiple linked files, I don't know how to reproduce it in a Python repository. I have used two dependencies like this, but the result only shows one.

# test_demo.py
import demo
import another_deps


def test_foo():
    assert demo.hello("World") == "Hello, World!"
    assert another_deps.another_hello() == "Hello from another_deps.py"
# demo.py
def hello(name):
    return "Hello, %s!" % name
# another_deps.py
def another_hello():
    return "Hello from another_deps.py"

The second bug is if we can't start the watcher, we can't stop it, and the error message is not good.

E5108: Error executing lua: /Users/scottming/Code/neotest/lua/nio/tasks.lua:95: Async task failed without callback: The coroutin
e failed with this message:
...ing/Code/neotest/lua/neotest/consumers/watch/watcher.lua:204: Expected lua number
stack traceback:
        [C]: in function 'nvim_del_autocmd'
        ...ing/Code/neotest/lua/neotest/consumers/watch/watcher.lua:204: in function 'stop_watch'
        ...ttming/Code/neotest/lua/neotest/consumers/watch/init.lua:133: in function 'stop'
        ...Code/neotest/lua/neotest/consumers/summary/component.lua:185: in function <...Code/neotest/lua/neotest/consumers/summ
ary/component.lua:183>
stack traceback:
        [C]: in function 'error'
        /Users/scottming/Code/neotest/lua/nio/tasks.lua:95: in function 'close_task'
        /Users/scottming/Code/neotest/lua/nio/tasks.lua:117: in function 'step'
        /Users/scottming/Code/neotest/lua/nio/tasks.lua:143: in function 'run'
        ...Code/neotest/lua/neotest/consumers/summary/component.lua:38: in function 'callback'
        ...ng/Code/neotest/lua/neotest/consumers/summary/canvas.lua:135: in function <...ng/Code/neotest/lua/neotest/consumers/s
ummary/canvas.lua:133>
Press ENTER or type command

The third bug is even I started the watcher, the test won't automatically run when saving deps files, like the above example, I tried modifying the demo.py and saving it, but the tests in test_demo.py won't run again automatically.

@rcarriga rcarriga force-pushed the feat/watch-consumer branch from 92badcc to e85ebd9 Compare April 15, 2023 09:46
@rcarriga rcarriga force-pushed the feat/nio-async-library branch from 500732c to 299101b Compare April 15, 2023 09:46
@rcarriga
Copy link
Copy Markdown
Collaborator Author

The second bug is if we can't start the watcher, we can't stop it, and the error message is not good.

I've added handling for watchers that don't start 👍

So the first bug I have no idea is can't start the watcher when there are multiple linked files, I don't know how to reproduce it in a Python repository. I have used two dependencies like this, but the result only shows one.

Interesting, the example you gave works for me. What LSP are you using? Are you able to see if the LSP responds correctly to the definition request?

@rcarriga rcarriga force-pushed the feat/nio-async-library branch from 299101b to da4f02a Compare April 15, 2023 10:12
Base automatically changed from feat/nio-async-library to master April 15, 2023 10:14
@scottming
Copy link
Copy Markdown
Contributor

function Watcher:_get_linked_files(path, root_path, args)
  logger.warn("symbol_queries is", args.symbol_queries)

  local symbols = lib.subprocess.enabled()
      and lib.subprocess.call(
        [[require("neotest.consumers.watch.watcher")._parse_symbols]],
        { path, args.symbol_queries }
      )
    or self._parse_symbols(path, args.symbol_queries)

There is a bug in these lines

it will convert the function to vim.NIL

WARN | 2023-04-15T19:10:53Z+0800 | ...ing/Code/neotest/lua/neotest/consumers/watch/watcher.lua:44 | symbol_queries is {
  elixir = <function 1>,
  lua = '        ;query\n        ;Captures module names in require calls\n        (function_call\n          name: ((identifier) @function (#eq? @function "require"))\n          arguments: (arguments (string) @symbol))\n      ',
  python = "        ;query\n        ;Captures imports and modules they're imported from\n        (import_from_statement (_ (identifier) @symbol))\n        (import_statement (_ (identifier) @symbol))\n      "
}
INFO | 2023-04-15T19:10:53Z+0800 | ...rs/scottming/Code/neotest/lua/neotest/lib/subprocess.lua:150 | CHILD | Received remote call 41 <function 1>
WARN | 2023-04-15T19:10:53Z+0800 | ...ing/Code/neotest/lua/neotest/consumers/watch/watcher.lua:26 | CHILD | type query is userdata for elixir vim.NIL

@rcarriga rcarriga force-pushed the feat/watch-consumer branch from e85ebd9 to 53df2f0 Compare April 15, 2023 13:41
@rcarriga
Copy link
Copy Markdown
Collaborator Author

Ah yep, adjusted the code to work in the subprocess 👍

@scottming
Copy link
Copy Markdown
Contributor

scottming commented Apr 16, 2023

I have found the reason why I cannot start the watcher. It is because there are some errors in the definitions of LSP that I am using, which do not return results on time(Fixed it and it works well now). I think maybe we need to set a timeout for this situation. For example, if we don't receive a reply after 1 second, we should cancel this request and print a log. This can solve the startup problem.

@scottming
Copy link
Copy Markdown
Contributor

scottming commented Apr 16, 2023

Another place where we can improve the experience is by providing a 'toggle_watch' function, so that we don't have to open the summary every time to watch or stop.

maybe:

_G.toggle_watch = function()
	local watch = require("neotest").watch
	local run = require("neotest").run
	local tree = run.get_tree_from_args({}, true)
	if not tree then
		print("no tree exists")
		return
	end

	local pos_id = tree:data().id
	local test
	if type(pos_id) == "string" then
		test = string.match(pos_id, "::(.*)")
	else
		test = pos_id
	end

	if watch.is_watching(pos_id) then
		watch.stop()
		require("notify")("Stoped watching: " .. test, "warn", { title = "neotest" })
	else
		watch.watch()
		require("notify")("Start watching: " .. test, "info", { title = "neotest" })
	end
end

@scottming
Copy link
Copy Markdown
Contributor

I have been experiencing this feature for a few days now, and overall my experience has been very good.

Comment on lines +102 to +128
function Watcher:_build_dependencies(root, paths, args, dependencies)
local count = 0
local worker = function()
while #paths > 0 do
local path = table.remove(paths)

if not dependencies[path] then
count = count + 1
dependencies[path] = {}
local path_results = self:_get_linked_files(path, root, args)
dependencies[path] = path_results

for _, p in ipairs(path_results) do
if not dependencies[p] then
paths[#paths + 1] = p
end
end
end
end
end
local num_workers = 4
local workers = {}
for _ = 1, num_workers do
workers[#workers + 1] = worker
end
nio.gather(workers)
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you describe this function's work details?

I am trying to understand why after building workers then, the thing works.

@rcarriga rcarriga force-pushed the feat/watch-consumer branch from 53df2f0 to 0d33224 Compare June 5, 2023 15:42
@rcarriga
Copy link
Copy Markdown
Collaborator Author

I haven't had much time to work on this but I've added a timeout for the LSP requests and a toggle function 😄

If there are no further issues I'll merge this soon

The lua language server doesn't show docs for function wrapped with
nio.create. This can be worked around by wrapping the functions after
declaring them normally
@rcarriga rcarriga merged commit 3313cb7 into master Jun 15, 2023
@rcarriga rcarriga deleted the feat/watch-consumer branch June 15, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants