- 
                Notifications
    
You must be signed in to change notification settings  - Fork 10
 
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.