Copilot commented on code in PR #263:
URL: https://github.com/apache/skywalking-eyes/pull/263#discussion_r2820125228
##########
commands/header_fix.go:
##########
@@ -28,9 +28,9 @@ import (
)
var FixCommand = &cobra.Command{
- Use: "fix",
+ Use: "fix [paths...]",
Aliases: []string{"f"},
- Long: "fix command walks the specified paths recursively and fix the
license header if the specified files don't have the license header.",
+ Long: "fix command walks the specified paths recursively and fixes
the license header if the specified files don't have the license header.
Accepts files, directories, and glob patterns. If no paths are specified, fixes
the current directory recursively as defined in the config file.",
Review Comment:
The updated documentation states that the fix command "Accepts files,
directories, and glob patterns" when command-line paths are provided. However,
the implementation (lines 40-46, not shown in this diff) doesn't actually
support directories or glob patterns when args are provided - it only passes
the args directly to header.Fix which expects individual file paths.
This documentation is misleading. Either the implementation needs to be
updated to support directories and glob patterns (similar to how check command
handles args by setting h.Paths = args), or the documentation should clarify
that directories and glob patterns are only supported through the config file,
not command-line arguments.
##########
pkg/header/check.go:
##########
@@ -150,6 +153,19 @@ func listFiles(config *ConfigHeader) ([]string, error) {
return fileList, nil
}
+func matchPaths(file string, patterns []string) bool {
+ for _, pattern := range patterns {
+ if pattern == "." {
+ pattern = "./"
+ }
+ matched, err := doublestar.Match(pattern, file)
+ if err == nil && matched {
+ return true
+ }
Review Comment:
The matchPaths function doesn't handle directory patterns consistently with
the existing tryMatchPatten function in config.go. When a user specifies a
directory pattern like "pkg/header/", it should match files within that
directory (e.g., "pkg/header/check.go"), but the current implementation using
only doublestar.Match will return false.
The tryMatchPatten function (lines 132-153 in pkg/header/config.go) includes
additional logic after doublestar.Match to handle directory patterns by
checking if the file path has the pattern as a prefix. This same logic should
be included in matchPaths to ensure consistent behavior between git and non-git
repositories.
Without this, users who specify directory paths (with or without trailing
slashes) will get different behavior in git vs non-git repositories.
```suggestion
}
// Fallback for directory patterns (e.g., "pkg/header/" or
"pkg/header")
// to behave consistently with tryMatchPatten in config.go.
if err == nil {
dirPattern := pattern
// Ensure the pattern ends with a path separator so it
clearly denotes a directory.
if !strings.HasSuffix(dirPattern, "/") &&
!strings.HasSuffix(dirPattern, string(os.PathSeparator)) {
dirPattern = dirPattern + "/"
}
// Normalize both paths to use '/' so prefix matching
is consistent across platforms.
normalizedDir := strings.ReplaceAll(dirPattern, "\\",
"/")
normalizedFile := strings.ReplaceAll(file, "\\", "/")
if strings.HasPrefix(normalizedFile, normalizedDir) {
return true
}
}
```
##########
pkg/header/check_test.go:
##########
@@ -273,3 +273,70 @@ func TestListFilesWithWorktreeDetachedHEAD(t *testing.T) {
t.Error("Expected to find files with valid commit")
}
}
+
+func TestMatchPaths(t *testing.T) {
+ tests := []struct {
+ name string
+ file string
+ patterns []string
+ expected bool
+ }{
+ {
+ name: "Exact file match",
+ file: "test.go",
+ patterns: []string{"test.go"},
+ expected: true,
+ },
+ {
+ name: "Glob pattern match",
+ file: "test.go",
+ patterns: []string{"*.go"},
+ expected: true,
+ },
+ {
+ name: "Double-star glob pattern match",
+ file: "pkg/header/check.go",
+ patterns: []string{"**/*.go"},
+ expected: true,
+ },
+ {
+ name: "Multiple patterns with match",
+ file: "test.go",
+ patterns: []string{"*.java", "*.go", "*.py"},
+ expected: true,
+ },
+ {
+ name: "Multiple patterns without match",
+ file: "test.go",
+ patterns: []string{"*.java", "*.py"},
+ expected: false,
+ },
+ {
+ name: "Directory pattern with trailing slash",
+ file: "pkg/header/check.go",
+ patterns: []string{"pkg/header/"},
+ expected: false,
Review Comment:
This test case expects that a directory pattern "pkg/header/" should NOT
match the file "pkg/header/check.go". However, this is inconsistent with the
existing tryMatchPatten function in pkg/header/config.go (lines 132-153), which
includes logic to match files within a specified directory.
In the non-git code path, when a user specifies "pkg/header/" as a path,
doublestar.Glob expands it to the directory, and walkFile recursively finds all
files within it. For consistency, the git code path should behave the same way
- files within "pkg/header/" should match the pattern "pkg/header/".
This test case should expect true, not false, and the matchPaths function
should be updated to include directory prefix matching logic similar to
tryMatchPatten.
```suggestion
expected: true,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
