- 
                Notifications
    
You must be signed in to change notification settings  - Fork 245
 
Read excluded patterns from configuration #637
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
          
     Open
      
      
            josuf107
  wants to merge
  5
  commits into
  fwcd:main
  
    
      
        
          
  
    
      Choose a base branch
      
     
    
      
        
      
      
        
          
          
        
        
          
            
              
              
              
  
           
        
        
          
            
              
              
           
        
       
     
  
        
          
            
          
            
          
        
       
    
      
from
josuf107:feature/configurable-exclusions
  
      
      
   
  
    
  
  
  
 
  
      
    base: main
Could not load branches
            
              
  
    Branch not found: {{ refName }}
  
            
                
      Loading
              
            Could not load tags
            
            
              Nothing to show
            
              
  
            
                
      Loading
              
            Are you sure you want to change the base?
            Some commits from the old base branch may be removed from the timeline,
            and old review comments may become outdated.
          
          
  
     Open
                    Changes from all commits
      Commits
    
    
            Show all changes
          
          
            5 commits
          
        
        Select commit
          Hold shift + click to select a range
      
      58c87bb
              
                Read excluded patterns from configuration
              
              
                josuf107 addcef3
              
                Handle directory patterns in source exclusions
              
              
                josuf107 50325f6
              
                Improvements to configurable source exclusions following review
              
              
                josuf107 1953984
              
                Fix comment text to match code
              
              
                josuf107 9786c21
              
                Merge branch 'main' into feature/configurable-exclusions
              
              
                josuf107 File filter
Filter by extension
Conversations
          Failed to load comments.   
        
        
          
      Loading
        
  Jump to
        
          Jump to file
        
      
      
          Failed to load files.   
        
        
          
      Loading
        
  Diff view
Diff view
There are no files selected for viewing
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              
        
          
          
            5 changes: 5 additions & 0 deletions
          
          5 
        
  shared/src/main/kotlin/org/javacs/kt/ExclusionsConfiguration.kt
  
  
      
      
   
        
      
      
    
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              | Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| package org.javacs.kt | ||
| 
     | 
||
| data class ExclusionsConfiguration( | ||
| var excludePatterns: List<String> = listOf(), // List of glob patterns | ||
| ) | 
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              
        
          
          
            59 changes: 59 additions & 0 deletions
          
          59 
        
  shared/src/test/kotlin/org/javacs/kt/SourceExclusionsTest.kt
  
  
      
      
   
        
      
      
    
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
              | Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| package org.javacs.kt | ||
| 
     | 
||
| import java.io.File | ||
| import java.nio.file.Files | ||
| import java.nio.file.Path | ||
| import java.nio.file.attribute.PosixFilePermissions | ||
| import org.hamcrest.Matchers.equalTo | ||
| import org.junit.After | ||
| import org.junit.Assert.assertTrue | ||
| import org.junit.Assert.assertFalse | ||
| import org.junit.Before | ||
| import org.junit.Test | ||
| 
     | 
||
| class SourceExclusionsTest { | ||
| private val workspaceRoots = listOf(File("/test/workspace1").toPath(), File("/test/workspace2").toPath()) | ||
| private var excludePatterns = listOf<String>() | ||
| 
     | 
||
| @Test | ||
| fun `test only default exclusions`() { | ||
| assertIncluded("/test/workspace1/src/main/kotlin/MyClass.kt") | ||
| assertIncluded("/test/workspace1/build/generated/blah.kt") | ||
| assertExcluded("/test/workspace1/.git") | ||
| assertExcluded("/test/workspace1/.git/HEAD") | ||
| } | ||
| 
     | 
||
| @Test | ||
| fun `test configured exclusions`() { | ||
| excludePatterns = listOf("build","junk") | ||
| assertIncluded("/test/workspace1/src/main/kotlin/MyClass.kt") | ||
| assertExcluded("/test/workspace1/build/generated/blah.kt") | ||
| assertExcluded("/test/workspace1/src/main/kotlin/junk/blah.kt") | ||
| } | ||
| 
     | 
||
| @Test | ||
| fun `test configured directory exclusions`() { | ||
| excludePatterns = listOf("build/dist/**") | ||
| assertIncluded("/test/workspace1/build/generated/blah.kt") | ||
| assertIncluded("/test/workspace1/blah/build/dist/blah.kt") | ||
| assertExcluded("/test/workspace1/build/dist/blah.kt") | ||
| } | ||
| 
     | 
||
| @Test | ||
| fun `test configured wildcard directory exclusions`() { | ||
| excludePatterns = listOf("**/build/dist/**", "build/dist/**") | ||
| assertIncluded("/test/workspace1/build/generated/blah.kt") | ||
| assertExcluded("/test/workspace1/blah/build/dist/blah.kt") | ||
| assertExcluded("/test/workspace1/build/dist/blah.kt") | ||
| } | ||
| 
     | 
||
| fun assertExcluded(path: String) { | ||
| val exclusions = SourceExclusions(workspaceRoots, ScriptsConfiguration(enabled = true, buildScriptsEnabled = true), ExclusionsConfiguration(excludePatterns=excludePatterns)) | ||
| assertFalse("Expected $path to be excluded by $excludePatterns", exclusions.isPathIncluded(File(path).toPath())) | ||
| } | ||
| 
     | 
||
| fun assertIncluded(path: String) { | ||
| val exclusions = SourceExclusions(workspaceRoots, ScriptsConfiguration(enabled = true, buildScriptsEnabled = true), ExclusionsConfiguration(excludePatterns=excludePatterns)) | ||
| assertTrue("Expected $path to be included by $excludePatterns", exclusions.isPathIncluded(File(path).toPath())) | ||
| } | ||
| } | 
  Add this suggestion to a batch that can be applied as a single commit.
  This suggestion is invalid because no changes were made to the code.
  Suggestions cannot be applied while the pull request is closed.
  Suggestions cannot be applied while viewing a subset of changes.
  Only one suggestion per line can be applied in a batch.
  Add this suggestion to a batch that can be applied as a single commit.
  Applying suggestions on deleted lines is not supported.
  You must change the existing code in this line in order to create a valid suggestion.
  Outdated suggestions cannot be applied.
  This suggestion has been applied or marked resolved.
  Suggestions cannot be applied from pending reviews.
  Suggestions cannot be applied on multi-line comments.
  Suggestions cannot be applied while the pull request is queued to merge.
  Suggestion cannot be applied right now. Please check back later.
  
    
  
    
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.
Any reason to change the defaults here? By the way, maybe we should move these hardcoded exclusions entirely to the config defaults?
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 thought about moving build and target to the defaults, but I figured people seem to be bumping into missing generated sources pretty often. It seems like it should find them by default. I'd be open to being convinced otherwise, but as far as I can tell the source exclusions mainly exist as a performance optimization to avoid walking more directories than necessary. Projects with build/target directories large enough to make a significant difference might expect needing to tweak settings a bit to get good performance, but it seems like people are continually surprised by the LSP not finding generated code by default.
I guess on that note I can help update configuration documentations if you don't mind pointing me to all the places that might want updating
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.
Oh right and on moving the good hardcoded exclusions (like node_modules and .git are often large and almost never will have kt source code), I didn't want people to have to include the good list in the case that they wanted to add something. I had the thought that maybe in the future we could add a supplemental configuration option to disable the defaults, but it probably won't come up that someone really needs to index node_modules or one of these scm directories.