Skip to content

Commit

Permalink
yamlfmt: fix standard mode exclude bug (#124)
Browse files Browse the repository at this point in the history
Due to a misunderstanding on my part, I swapped standard filepath
collection mode to calculate exclusions using absolute paths. This
worked because of the way tests were written, but it does not work in
most real world scenarios and it slipped by. I will be able to catch
this better when I implement proper integration testing.

This PR removes the absolute paths from the calculation, and adds a doc
entry to recommend that paths are always specified relative to the
command working directory.
  • Loading branch information
braydonk authored Jun 7, 2023
1 parent 8cc9885 commit 908b190
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 9 deletions.
4 changes: 4 additions & 0 deletions docs/paths.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,7 @@ In standard path mode, the you can specify a file or directory path directly. If
## Doublestar

In Doublestar mode, paths are specified using the format explained in the [doublestar](https://github.com/bmatcuk/doublestar) package. It is almost identical to bash and git's style of glob pattern specification.

## Include and Exclude

In both modes, `yamlfmt` will allow you to configure include and exclude paths. These can be paths to files in Standard or Doublestar modes, paths to directories in Standard mode, and valid doublestar patterns in Doublestar mode. These paths should be specified **relative to the working directory of `yamlfmt`**. They will work as absolute paths if both the includes and excludes are specified as absolute paths or if both are relative paths, however it will not work as expected if they are mixed together. It usually easier to reason about includes and excludes when always specifying both as relative paths from the directory `yamlfmt` is going to be run in.
9 changes: 2 additions & 7 deletions path_collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,19 +52,14 @@ func (c *FilepathCollector) CollectPaths() ([]string, error) {
continue
}

absExclPath, err := filepath.Abs(exclPath)
if err != nil {
return nil, err
}

if info.IsDir() {
for foundPath := range pathsFoundSet {
if strings.HasPrefix(foundPath, absExclPath) {
if strings.HasPrefix(foundPath, exclPath) {
pathsToFormat.Remove(foundPath)
}
}
} else {
pathsToFormat.Remove(absExclPath)
pathsToFormat.Remove(exclPath)
}
}

Expand Down
24 changes: 22 additions & 2 deletions path_collector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,8 @@ func TestFilepathCollector(t *testing.T) {
},
},
{
name: "exclude directory",
name: "exclude directory",
changeToTempDir: true,
files: []tempfile.Path{
{FileName: "x.yml"},
{FileName: "y.yml"},
Expand Down Expand Up @@ -159,6 +160,25 @@ func TestFilepathCollector(t *testing.T) {
"a/x.yaml": {},
},
},
{
name: "don't get files with wrong extension",
files: []tempfile.Path{
{FileName: "x.yml"},
{FileName: "y.yaml"},
{FileName: "z.json"},
},
includePatterns: testPatterns{
{pattern: ""}, // with the test this functionally means the whole temp dir
},
extensions: []string{
"yaml",
"yml",
},
expectedFiles: collections.Set[string]{
"x.yml": {},
"y.yaml": {},
},
},
}.runAll(t, useFilepathCollector)
}

Expand Down Expand Up @@ -338,7 +358,7 @@ func (tc testCase) run(t *testing.T, makeCollector makeCollectorFunc) {
collector := makeCollector(tc, tempPath)
paths, err := collector.CollectPaths()
if err != nil {
t.Fatalf("CollectDoublestarPathsToFormat failed: %v", err)
t.Fatalf("Test case failed: %v", err)
}

filesToFormat := collections.Set[string]{}
Expand Down

0 comments on commit 908b190

Please sign in to comment.