-
Notifications
You must be signed in to change notification settings - Fork 14
feat: draft file filtering [PS-105] #487
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
base: main
Are you sure you want to change the base?
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| ) | ||
|
|
||
| type FileFilterStrategy interface { | ||
| FilterOut(path string) bool |
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.
nitpick: I'd call it Filter
| ignores *gitignore.GitIgnore | ||
| } | ||
|
|
||
| func NewGlobFileFilter(path string, ruleFiles []string, logger *zerolog.Logger) (FileFilterStrategy, error) { |
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'd call it GitIgnoreFileFilter as it explicitly uses gitignore syntax, I think.
| logger *zerolog.Logger | ||
| max_threads int64 | ||
| path string | ||
| defaultRules []string |
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.
this may not be needed anymore, as it could be a strategy, right?
| "gopkg.in/yaml.v3" | ||
| ) | ||
|
|
||
| type FileFilterStrategy interface { |
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.
Potentially more go idiomatic different name (don't care much): Filterable
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.
Something to consider would be something like this:
func Filter[T any](items []T, predicate func(T) bool) []T {
var result []T
for _, item := range items {
if predicate(item) {
result = append(result, item)
}
}
return result
}
| if filter.FilterOut(f) { | ||
| keepFile = false | ||
| break | ||
| } | ||
| } |
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.
Yep, exactly like I was thinking how it could/would work.
| if !globPatternMatcher.MatchesPath(f) { | ||
| // filesToFilter that do not match the filter list are excluded | ||
| keepFile := true | ||
| for _, filter := range fw.filterStrategies { |
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.
A thought: we could also have a AggregateFileStrategy that delegates to sub-filters and encapsulates the logic.
| b.ResetTimer() | ||
| for n := 0; n < b.N; n++ { | ||
| fileFilter := NewFileFilter(rootDir, &log.Logger, WithThreadNumber(runtime.NumCPU())) | ||
| globFileFilter, err := NewGlobFileFilter(rootDir, ruleFiles, &log.Logger) |
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.
Probably we should leave that test to ensure that clients that use the NewFileFilter functions right now have a smooth upgrade path.
| globFileFilter, err := NewGlobFileFilter(rootDir, ruleFiles, &log.Logger) | ||
| assert.NoError(b, err) | ||
|
|
||
| fileFilter := NewFileFilter(rootDir, &log.Logger, WithFileFilterStrategies([]FileFilterStrategy{globFileFilter}), WithThreadNumber(runtime.NumCPU())) |
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.
this should probably be a new test
No description provided.