Skip to content

Conversation

@alina-d-m
Copy link

No description provided.

@snyk-io
Copy link

snyk-io bot commented Nov 20, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-io
Copy link

snyk-io bot commented Nov 20, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

)

type FileFilterStrategy interface {
FilterOut(path string) bool
Copy link
Contributor

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) {
Copy link
Contributor

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
Copy link
Contributor

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 {
Copy link
Contributor

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

Copy link
Contributor

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
}

Comment on lines +178 to +182
if filter.FilterOut(f) {
keepFile = false
break
}
}
Copy link
Contributor

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 {
Copy link
Contributor

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)
Copy link
Contributor

@bastiandoetsch bastiandoetsch Nov 20, 2025

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()))
Copy link
Contributor

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

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