Skip to content

Commit b11efe3

Browse files
sethvargosethvargo
authored
Refactor internals to keep track of filenames (#104)
This also centralizes the YAML writing into a single place (reducing boilerplate in commands) and cleans up some unnecessary variable duplication in tests.
1 parent 2ebf532 commit b11efe3

25 files changed

Lines changed: 161 additions & 204 deletions

‎command/check.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,12 +60,10 @@ func (c *CheckCommand) Run(ctx context.Context, originalArgs []string) error {
6060
return err
6161
}
6262

63-
fsys := os.DirFS(".")
64-
65-
files, err := loadYAMLFiles(fsys, args)
63+
loadResult, err := loadYAMLFiles(os.DirFS("."), args)
6664
if err != nil {
6765
return err
6866
}
6967

70-
return parser.Check(ctx, par, files.nodes())
68+
return parser.Check(ctx, par, loadResult.nodes())
7169
}

‎command/cmd/gen/main.go‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,19 @@ func folderRoot() string {
5656
func buildTopLevelHelp() string {
5757
var w strings.Builder
5858

59+
commands := make(map[string]command.Command, len(command.Commands))
60+
for name, command := range command.Commands {
61+
// Ignore hidden commands
62+
if _, ok := command.(interface{ Hidden() }); ok {
63+
continue
64+
}
65+
66+
commands[name] = command
67+
}
68+
5969
longest := 0
60-
names := make([]string, 0, len(command.Commands))
61-
for name := range command.Commands {
70+
names := make([]string, 0, len(commands))
71+
for name := range commands {
6272
names = append(names, name)
6373
if l := len(name); l > longest {
6474
longest = l

‎command/command.go‎

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,15 @@ import (
99
"fmt"
1010
"io/fs"
1111
"os"
12+
"path/filepath"
1213
"slices"
1314
"strconv"
1415
"strings"
1516

1617
// Using banydonk/yaml instead of the default yaml pkg because the default
1718
// pkg incorrectly escapes unicode. https://github.com/go-yaml/yaml/issues/737
1819
"github.com/braydonk/yaml"
20+
"github.com/sethvargo/ratchet/internal/atomic"
1921
"github.com/sethvargo/ratchet/internal/version"
2022
)
2123

@@ -108,7 +110,6 @@ func marshalYAML(m *yaml.Node) (string, error) {
108110
}
109111

110112
type loadResult struct {
111-
path string
112113
node *yaml.Node
113114
contents string
114115
newlines []int
@@ -136,18 +137,18 @@ func (r *loadResult) marshalYAML() (string, error) {
136137
return strings.Join(lines, "\n"), nil
137138
}
138139

139-
type loadResults []*loadResult
140+
type loadResults map[string]*loadResult
140141

141-
func (r loadResults) nodes() []*yaml.Node {
142-
n := make([]*yaml.Node, 0, len(r))
143-
for _, v := range r {
144-
n = append(n, v.node)
142+
func (r loadResults) nodes() map[string]*yaml.Node {
143+
m := make(map[string]*yaml.Node, len(r))
144+
for name, lr := range r {
145+
m[name] = lr.node
145146
}
146-
return n
147+
return m
147148
}
148149

149150
func loadYAMLFiles(fsys fs.FS, paths []string) (loadResults, error) {
150-
r := make(loadResults, 0, len(paths))
151+
r := make(loadResults, len(paths))
151152

152153
for _, pth := range paths {
153154
pth = strings.TrimPrefix(pth, "./")
@@ -172,17 +173,47 @@ func loadYAMLFiles(fsys fs.FS, paths []string) (loadResults, error) {
172173

173174
newlines := computeNewlineTargets(string(contents), remarshaled)
174175

175-
r = append(r, &loadResult{
176-
path: pth,
176+
if _, ok := r[pth]; ok {
177+
return nil, fmt.Errorf("internal error: entry already exists for %q: %v", pth, r)
178+
}
179+
180+
r[pth] = &loadResult{
177181
node: &node,
178182
contents: string(contents),
179183
newlines: newlines,
180-
})
184+
}
181185
}
182186

183187
return r, nil
184188
}
185189

190+
func (r loadResults) writeYAMLFiles(outPath string) error {
191+
var merr error
192+
193+
for pth, f := range r {
194+
outFile := outPath
195+
if strings.HasSuffix(outPath, "/") {
196+
outFile = filepath.Join(outPath, pth)
197+
}
198+
if outFile == "" {
199+
outFile = pth
200+
}
201+
202+
final, err := f.marshalYAML()
203+
if err != nil {
204+
merr = errors.Join(merr, fmt.Errorf("failed to marshal yaml for %s: %w", pth, err))
205+
continue
206+
}
207+
208+
if err := atomic.Write(pth, outFile, strings.NewReader(final)); err != nil {
209+
merr = errors.Join(merr, fmt.Errorf("failed to save file %s: %w", outFile, err))
210+
continue
211+
}
212+
}
213+
214+
return merr
215+
}
216+
186217
func computeNewlineTargets(before, after string) []int {
187218
before = strings.TrimPrefix(before, "---\n")
188219

‎command/command_test.go‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -31,25 +31,23 @@ func Test_loadYAMLFiles(t *testing.T) {
3131
}
3232

3333
for input, expected := range cases {
34-
inputFilename, expectedFilename := input, expected
35-
36-
t.Run(inputFilename, func(t *testing.T) {
34+
t.Run(input, func(t *testing.T) {
3735
t.Parallel()
3836

39-
files, err := loadYAMLFiles(fsys, []string{inputFilename})
37+
files, err := loadYAMLFiles(fsys, []string{input})
4038
if err != nil {
4139
t.Fatal(err)
4240
}
4341

44-
got, err := files[0].marshalYAML()
42+
got, err := files[input].marshalYAML()
4543
if err != nil {
4644
t.Fatal(err)
4745
}
4846

49-
if expectedFilename == "" {
50-
expectedFilename = inputFilename
47+
if expected == "" {
48+
expected = input
5149
}
52-
want, err := fsys.(fs.ReadFileFS).ReadFile(expectedFilename)
50+
want, err := fsys.(fs.ReadFileFS).ReadFile(expected)
5351
if err != nil {
5452
t.Fatal(err)
5553
}
@@ -115,8 +113,6 @@ func Test_computeNewlineTargets_simple(t *testing.T) {
115113
}
116114

117115
for _, tc := range cases {
118-
tc := tc
119-
120116
t.Run(tc.name, func(t *testing.T) {
121117
t.Parallel()
122118

@@ -199,8 +195,6 @@ test-code-job1:
199195
}
200196

201197
for _, tc := range cases {
202-
tc := tc
203-
204198
t.Run(tc.name, func(t *testing.T) {
205199
t.Parallel()
206200

@@ -215,7 +209,7 @@ test-code-job1:
215209
t.Fatal(err)
216210
}
217211

218-
f := r[0]
212+
f := r["file.yml"]
219213
if diff := cmp.Diff(f.newlines, tc.want); diff != "" {
220214
t.Errorf("unexpected newlines diff (+got, -want):\n%s", diff)
221215
}

‎command/pin.go‎

Lines changed: 5 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,8 @@ import (
55
"flag"
66
"fmt"
77
"os"
8-
"path/filepath"
98
"strings"
109

11-
"github.com/sethvargo/ratchet/internal/atomic"
1210
"github.com/sethvargo/ratchet/internal/concurrency"
1311
"github.com/sethvargo/ratchet/parser"
1412
"github.com/sethvargo/ratchet/resolver"
@@ -78,38 +76,21 @@ func (c *PinCommand) Run(ctx context.Context, originalArgs []string) error {
7876
return fmt.Errorf("failed to create resolver: %w", err)
7977
}
8078

81-
fsys := os.DirFS(".")
82-
83-
files, err := loadYAMLFiles(fsys, args)
79+
loadResult, err := loadYAMLFiles(os.DirFS("."), args)
8480
if err != nil {
8581
return err
8682
}
8783

88-
if len(files) > 1 && c.flagOut != "" && !strings.HasSuffix(c.flagOut, "/") {
84+
if len(loadResult) > 1 && c.flagOut != "" && !strings.HasSuffix(c.flagOut, "/") {
8985
return fmt.Errorf("-out must be a directory when pinning multiple files")
9086
}
9187

92-
if err := parser.Pin(ctx, res, par, files.nodes(), c.flagConcurrency); err != nil {
88+
if err := parser.Pin(ctx, res, par, loadResult.nodes(), c.flagConcurrency); err != nil {
9389
return fmt.Errorf("failed to pin refs: %w", err)
9490
}
9591

96-
for _, f := range files {
97-
outFile := c.flagOut
98-
if strings.HasSuffix(c.flagOut, "/") {
99-
outFile = filepath.Join(c.flagOut, f.path)
100-
}
101-
if outFile == "" {
102-
outFile = f.path
103-
}
104-
105-
final, err := f.marshalYAML()
106-
if err != nil {
107-
return fmt.Errorf("failed to marshal yaml for %s: %w", f.path, err)
108-
}
109-
110-
if err := atomic.Write(f.path, outFile, strings.NewReader(final)); err != nil {
111-
return fmt.Errorf("failed to save file %s: %w", outFile, err)
112-
}
92+
if err := loadResult.writeYAMLFiles(c.flagOut); err != nil {
93+
return fmt.Errorf("failed to save files: %w", err)
11394
}
11495

11596
return nil

‎command/unpin.go‎

Lines changed: 5 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,8 @@ import (
55
"flag"
66
"fmt"
77
"os"
8-
"path/filepath"
98
"strings"
109

11-
"github.com/sethvargo/ratchet/internal/atomic"
1210
"github.com/sethvargo/ratchet/parser"
1311
)
1412

@@ -62,38 +60,21 @@ func (c *UnpinCommand) Run(ctx context.Context, originalArgs []string) error {
6260
return fmt.Errorf("failed to parse flags: %w", err)
6361
}
6462

65-
fsys := os.DirFS(".")
66-
67-
files, err := loadYAMLFiles(fsys, args)
63+
loadResult, err := loadYAMLFiles(os.DirFS("."), args)
6864
if err != nil {
6965
return err
7066
}
7167

72-
if len(files) > 1 && c.flagOut != "" && !strings.HasSuffix(c.flagOut, "/") {
68+
if len(loadResult) > 1 && c.flagOut != "" && !strings.HasSuffix(c.flagOut, "/") {
7369
return fmt.Errorf("-out must be a directory when pinning multiple files")
7470
}
7571

76-
if err := parser.Unpin(ctx, files.nodes()); err != nil {
72+
if err := parser.Unpin(ctx, loadResult.nodes()); err != nil {
7773
return fmt.Errorf("failed to pin refs: %w", err)
7874
}
7975

80-
for _, f := range files {
81-
outFile := c.flagOut
82-
if strings.HasSuffix(c.flagOut, "/") {
83-
outFile = filepath.Join(c.flagOut, f.path)
84-
}
85-
if outFile == "" {
86-
outFile = f.path
87-
}
88-
89-
final, err := f.marshalYAML()
90-
if err != nil {
91-
return fmt.Errorf("failed to marshal yaml for %s: %w", f.path, err)
92-
}
93-
94-
if err := atomic.Write(f.path, outFile, strings.NewReader(final)); err != nil {
95-
return fmt.Errorf("failed to save file %s: %w", outFile, err)
96-
}
76+
if err := loadResult.writeYAMLFiles(c.flagOut); err != nil {
77+
return fmt.Errorf("failed to save files: %w", err)
9778
}
9879

9980
return nil

‎command/update.go‎

Lines changed: 6 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,8 @@ import (
55
"flag"
66
"fmt"
77
"os"
8-
"path/filepath"
98
"strings"
109

11-
"github.com/sethvargo/ratchet/internal/atomic"
1210
"github.com/sethvargo/ratchet/parser"
1311
"github.com/sethvargo/ratchet/resolver"
1412
)
@@ -67,42 +65,25 @@ func (c *UpdateCommand) Run(ctx context.Context, originalArgs []string) error {
6765
return fmt.Errorf("failed to create resolver: %w", err)
6866
}
6967

70-
fsys := os.DirFS(".")
71-
72-
files, err := loadYAMLFiles(fsys, args)
68+
loadResult, err := loadYAMLFiles(os.DirFS("."), args)
7369
if err != nil {
7470
return err
7571
}
7672

77-
if len(files) > 1 && c.flagOut != "" && !strings.HasSuffix(c.flagOut, "/") {
73+
if len(loadResult) > 1 && c.flagOut != "" && !strings.HasSuffix(c.flagOut, "/") {
7874
return fmt.Errorf("-out must be a directory when pinning multiple files")
7975
}
8076

81-
if err := parser.Unpin(ctx, files.nodes()); err != nil {
77+
if err := parser.Unpin(ctx, loadResult.nodes()); err != nil {
8278
return fmt.Errorf("failed to pin refs: %w", err)
8379
}
8480

85-
if err := parser.Pin(ctx, res, par, files.nodes(), c.flagConcurrency); err != nil {
81+
if err := parser.Pin(ctx, res, par, loadResult.nodes(), c.flagConcurrency); err != nil {
8682
return fmt.Errorf("failed to pin refs: %w", err)
8783
}
8884

89-
for _, f := range files {
90-
outFile := c.flagOut
91-
if strings.HasSuffix(c.flagOut, "/") {
92-
outFile = filepath.Join(c.flagOut, f.path)
93-
}
94-
if outFile == "" {
95-
outFile = f.path
96-
}
97-
98-
final, err := f.marshalYAML()
99-
if err != nil {
100-
return fmt.Errorf("failed to marshal yaml for %s: %w", f.path, err)
101-
}
102-
103-
if err := atomic.Write(f.path, outFile, strings.NewReader(final)); err != nil {
104-
return fmt.Errorf("failed to save file %s: %w", outFile, err)
105-
}
85+
if err := loadResult.writeYAMLFiles(c.flagOut); err != nil {
86+
return fmt.Errorf("failed to save files: %w", err)
10687
}
10788

10889
return nil

0 commit comments

Comments
 (0)