Skip to content

Conversation

@Dynge
Copy link
Contributor

@Dynge Dynge commented Oct 17, 2025

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.

@Dynge
Copy link
Contributor Author

Dynge commented Oct 18, 2025

I'll rebase this MR later. Shouldn't really have to built upon the other change 😅

@Dynge Dynge force-pushed the mdl/push-zrzwtsuknptk branch from 40030ae to 9343e76 Compare October 18, 2025 08:21
@Dynge
Copy link
Contributor Author

Dynge commented Oct 18, 2025

I'll rebase this MR later. Shouldn't really have to built upon the other change 😅

Rebased.

@liskin liskin mentioned this pull request Oct 18, 2025
@Nsidorenco
Copy link
Owner

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:

  1. Start at root and descend.
  2. For any child directory, if filter dir returns true then do not scan any sub-directories of that directory.
  3. Repeat the descend logic as long as some child directory is not filtered away.

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

@Dynge
Copy link
Contributor Author

Dynge commented Oct 19, 2025

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.

@Dynge Dynge force-pushed the mdl/push-zrzwtsuknptk branch 2 times, most recently from b242b92 to f57601c Compare October 19, 2025 13:16
@Dynge
Copy link
Contributor Author

Dynge commented Oct 19, 2025

Updated it to look for parent first, and then only if that fails check child directories.

@Dynge Dynge force-pushed the mdl/push-zrzwtsuknptk branch 2 times, most recently from 18c79ca to 0d9490f Compare October 19, 2025 13:30
@Dynge
Copy link
Contributor Author

Dynge commented Oct 19, 2025 via email

@liskin
Copy link
Contributor

liskin commented Oct 19, 2025

It may be nothing but I can't help but feel that we are doing a lot of redundant searches here.

Seems like, yeah:

DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'
DEBUG | 2025-10-19T17:41:15Z+0100 | .../bundle/start/neotest-vstest/lua/neotest-vstest/init.lua:188 | neotest-vstest: file 'tools' is not a part of the solution '<redacted>.sln'

I get a lot of this :-)

@Dynge Dynge force-pushed the mdl/push-zrzwtsuknptk branch 2 times, most recently from 8fea987 to ff306db Compare October 19, 2025 18:14
@Nsidorenco
Copy link
Owner

Nsidorenco commented Oct 19, 2025

It may be nothing but I can't help but feel that we are doing a lot of redundant searches here.

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.

vim.fs.relpath was added in neovim 0.11 so I'd be interested to see if we could figure out a solution that does not involve scanning all the directories, perhaps at the cost of bumping the minimal required neovim version of the plugin.

@Nsidorenco Nsidorenco changed the base branch from main to tests/subfolder-detection October 19, 2025 19:10
@Nsidorenco Nsidorenco changed the base branch from tests/subfolder-detection to main October 19, 2025 19:11
@Dynge Dynge force-pushed the mdl/push-zrzwtsuknptk branch from ff306db to b21f13d Compare October 19, 2025 19:46
@Dynge
Copy link
Contributor Author

Dynge commented Oct 19, 2025

It may be nothing but I can't help but feel that we are doing a lot of redundant searches here.

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.

vim.fs.relpath was added in neovim 0.11 so I'd be interested to see if we could figure out a solution that does not involve scanning all the directories, perhaps at the cost of bumping the minimal required neovim version of the plugin.

Ive just updated the PR to use string matching.

The point is that if i have path a/b/c and i need to know if I can filter it away or not, i need to check if a/b/c is either a subdir of a/b/c or if a/b/c is a subdir of a project.

ProjectDir  DirUnderTest    
a/b         a/b/c           ProjectDir contained in DirUnderTest
a/b/c/d     a/b/c           DirUnderTest contained in ProjectDir

@Dynge Dynge force-pushed the mdl/push-zrzwtsuknptk branch 2 times, most recently from 20887e1 to 2f5ad84 Compare October 19, 2025 20:05
@liskin
Copy link
Contributor

liskin commented Oct 19, 2025

vim.fs.relpath was added in neovim 0.11 so I'd be interested to see if we could figure out a solution that does not involve scanning all the directories, perhaps at the cost of bumping the minimal required neovim version of the plugin.

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) ~= nil

instead of the

if fullpath:find(project_dir, 1, true) or project_dir:find(fullpath, 1, true) then
  return true
end
return false

and 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

@liskin
Copy link
Contributor

liskin commented Oct 19, 2025

If, for whatever reason, we want to keep using the string match, then there's vim.startswith that has been there for years. Probably a bit more reliable than string.find which returns non-nil whenever the needle is anywhere in the haystack.

@Dynge
Copy link
Contributor Author

Dynge commented Oct 20, 2025

If, for whatever reason, we want to keep using the string match, then there's vim.startswith that has been there for years. Probably a bit more reliable than string.find which returns non-nil whenever the needle is anywhere in the haystack.

I kind of like the string matching. I cant think of any problematic edge-cases off the top of my head.
I like using startswith instead, its a little clearer what we are actually looking for :)

What do you think @Nsidorenco ?

@liskin
Copy link
Contributor

liskin commented Oct 20, 2025

I kind of like the string matching. I cant think of any problematic edge-cases off the top of my head. I like using startswith instead, its a little clearer what we are actually looking for :)

Well, /ab/cd/ef starts with /ab/c, even though neither is a parent of the other, so that's one edge-case. Not a contrived one, either, as "test-harness" starts with "test", and that's a potentially realistic example maybe? You could solve that by adding /, but then we're back at what @Nsidorenco said earlier:

The problem back then was that neovim had no real utilities for comparing paths and window-style pathing just have so many edge-cases.

(I understood that as meaning "Windows-style" rather than "window-style".)

@Dynge
Copy link
Contributor Author

Dynge commented Oct 20, 2025

Well, /ab/cd/ef starts with /ab/c,

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. :)

@Nsidorenco
Copy link
Owner

From what I've see vim.fs.relpath seems like a good solution.

Did any one of you use windows when testing out the functionality? I don't have a windows machine myself.

@liskin
Copy link
Contributor

liskin commented Oct 20, 2025

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 🙂

@Dynge Dynge force-pushed the mdl/push-zrzwtsuknptk branch from 2f5ad84 to a33cb74 Compare October 20, 2025 17:18
@Nsidorenco
Copy link
Owner

Is it still necessary to use vim.fs.normalize after the change? Otherwise I think we’re good to go

@Dynge Dynge force-pushed the mdl/push-zrzwtsuknptk branch from a33cb74 to 2c5a3cc Compare October 20, 2025 19:57
@Dynge
Copy link
Contributor Author

Dynge commented Oct 20, 2025

Is it still necessary to use vim.fs.normalize after the change? Otherwise I think we’re good to go

Removed normalize calls.

string.format(
"neotest-vstest: file '%s' is not a part of the solution '%s'",
rel_path,
solution_info.solution_file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
solution_info.solution_file
solution_info and solution_info.solution_file or ""

(lua_ls says "Need check nil.")

Copy link
Contributor

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?

Copy link
Contributor Author

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.

@Dynge Dynge force-pushed the mdl/push-zrzwtsuknptk branch from 2c5a3cc to f61edfe Compare October 21, 2025 05:42
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.
@Dynge Dynge force-pushed the mdl/push-zrzwtsuknptk branch from f61edfe to f03fe9d Compare October 21, 2025 05:46
@Nsidorenco
Copy link
Owner

I’ll go ahead and merge this now - really happy with how it ended up. Great work, both of you 🙌🏻

@Nsidorenco Nsidorenco merged commit 83cf9a1 into Nsidorenco:main Oct 21, 2025
6 checks passed
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.

3 participants