-
Notifications
You must be signed in to change notification settings - Fork 14
fix(dir-filter): look for first parent project #47
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
|
I'll rebase this MR later. Shouldn't really have to built upon the other change 😅 |
40030ae to
9343e76
Compare
Rebased. |
|
You're right that we should be looking for a parent project but we also need to be checking for child projects for the filtering to be working correctly. The filtering implementation of neotest works like so:
If we only look "upwards" then we might end the process prematurely since there can be many folders between the root and the first project file |
|
Ah makes sense. I have never worked in a project where the csproj files are more than 1 layer deep, so I did not even think about that. |
b242b92 to
f57601c
Compare
|
Updated it to look for parent first, and then only if that fails check child directories. |
18c79ca to
0d9490f
Compare
|
I can change that.
It may be nothing but I can't help but feel that we are doing a lot of redundant searches here. 😅
|
Seems like, yeah: I get a lot of this :-) |
8fea987 to
ff306db
Compare
Agreed. My first implementation of this was actually not to recursively scan for project files but rather just get the list of all projects in the solution and then check if the direction we are currently filtering is a subpath of any project path. The problem back then was that neovim had no real utilities for comparing paths and window-style pathing just have so many edge-cases.
|
ff306db to
b21f13d
Compare
Ive just updated the PR to use string matching. The point is that if i have path |
20887e1 to
2f5ad84
Compare
I tested the latest (2f5ad84) version of this PR with return vim.fs.relpath(fullpath, project_dir) ~= nil or vim.fs.relpath(project_dir, fullpath) ~= nilinstead of the if fullpath:find(project_dir, 1, true) or project_dir:find(fullpath, 1, true) then
return true
end
return falseand it seems to work fine. So if you're willing to bump the min to nvim 0.11, that's the solution I'd use |
|
If, for whatever reason, we want to keep using the string match, then there's |
I kind of like the string matching. I cant think of any problematic edge-cases off the top of my head. What do you think @Nsidorenco ? |
Well,
(I understood that as meaning "Windows-style" rather than "window-style".) |
Silly me. Get completely blindsided by this edge case xD. That does indeed not sound all that unlikely. I have no issue using vim.fs.relpath and thus requiring a newer nvim version. :) |
|
From what I've see Did any one of you use windows when testing out the functionality? I don't have a windows machine myself. |
No, just Linux. I have a Windows VM somewhere but I don't even know how one installs neovim there so it'd take me more time than I have to go and try. I think it should be safe to assume relpath does the right thing. If not someone will file an issue sooner or later 🙂 |
2f5ad84 to
a33cb74
Compare
|
Is it still necessary to use |
a33cb74 to
2c5a3cc
Compare
Removed normalize calls. |
| string.format( | ||
| "neotest-vstest: file '%s' is not a part of the solution '%s'", | ||
| rel_path, | ||
| solution_info.solution_file |
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.
| solution_info.solution_file | |
| solution_info and solution_info.solution_file or "" |
(lua_ls says "Need check nil.")
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.
Actually, this is a bit of a hack. Probably better to adjust the code above that does if solution_dir then local solution_info … to instead be something like local solution_info = solution_dir and solution and dotnet_utils.get_solution_info(solution) and then if solution_info then …. Or something like that. Do we even need to check solution_dir at all?
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.
Yeah. I agree that there are some type checking errors, however I think a change to this would be out of scope for this PR :)
Could be made in another small refactoring PR.
2c5a3cc to
f61edfe
Compare
This change updates the dir_filter to use vim.fs.relpath to detect if the file is a decendant or parent of a relevant project from the solution. This change should fix the error as well as be more performant because we do not need to scan the directory for every file checked.
f61edfe to
f03fe9d
Compare
|
I’ll go ahead and merge this now - really happy with how it ended up. Great work, both of you 🙌🏻 |
In order to see if the directory should be scanned we need to check upward for the first project (assuming that no file can be contained within two or more projects at the same time).
Furthermore Ive changed some variable names and added a log if the directory is ignored because it is determined not to be from the solution project.