Skip to content

Commit a04214c

Browse files
committed
List - Address review feedback for --json output
- Filter non-job lines (comments, env vars) from JSON output so it matches the table output behavior. Creates a shallow copy of each crontab with only job lines before marshaling. - Return error from printListAsJSON instead of calling os.Exit directly, making the error path testable. - Check w.Write return value instead of discarding it. - Add TestPrintListAsJSONExcludesNonJobLines test. https://claude.ai/code/session_01K2duHV2B2CrFFVC5SYU6pH
1 parent ef72f58 commit a04214c

File tree

2 files changed

+69
-16
lines changed

2 files changed

+69
-16
lines changed

cmd/list.go

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,10 @@ Example:
4242
}
4343

4444
if printJSON {
45-
printListAsJSON(os.Stdout, crontabs)
45+
if err := printListAsJSON(os.Stdout, crontabs); err != nil {
46+
fmt.Fprintf(os.Stderr, "Error encoding JSON: %s\n", err)
47+
os.Exit(1)
48+
}
4649
} else {
4750
printListAsTable(os.Stdout, crontabs)
4851
}
@@ -123,24 +126,30 @@ func printListAsTable(w io.Writer, crontabs []*lib.Crontab) {
123126
}
124127

125128
// printListAsJSON marshals crontabs to JSON and writes to w.
126-
// Only crontabs with job lines are included.
127-
func printListAsJSON(w io.Writer, crontabs []*lib.Crontab) {
128-
var output []*lib.Crontab
129+
// Only crontabs with job lines are included, and within each crontab
130+
// only job lines are emitted (matching the table output behavior).
131+
func printListAsJSON(w io.Writer, crontabs []*lib.Crontab) error {
132+
var output []lib.Crontab
129133
for _, crontab := range crontabs {
130-
if len(jobLines(crontab)) > 0 {
131-
output = append(output, crontab)
134+
jobs := jobLines(crontab)
135+
if len(jobs) == 0 {
136+
continue
132137
}
138+
// Shallow copy with only job lines so comments/env vars are excluded
139+
filtered := *crontab
140+
filtered.Lines = jobs
141+
output = append(output, filtered)
133142
}
134143

135144
if output == nil {
136-
output = []*lib.Crontab{}
145+
output = []lib.Crontab{}
137146
}
138147

139148
data, err := json.MarshalIndent(output, "", " ")
140149
if err != nil {
141-
fmt.Fprintf(os.Stderr, "Error encoding JSON: %s\n", err)
142-
os.Exit(1)
150+
return err
143151
}
144152
data = append(data, '\n')
145-
w.Write(data)
153+
_, err = w.Write(data)
154+
return err
146155
}

cmd/list_test.go

Lines changed: 50 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,9 @@ func TestPrintListAsJSONValidOutput(t *testing.T) {
8383
}
8484

8585
var buf bytes.Buffer
86-
printListAsJSON(&buf, crontabs)
86+
if err := printListAsJSON(&buf, crontabs); err != nil {
87+
t.Fatalf("printListAsJSON returned error: %v", err)
88+
}
8789

8890
if buf.Len() == 0 {
8991
t.Fatal("printListAsJSON produced no output")
@@ -102,7 +104,9 @@ func TestPrintListAsJSONStructure(t *testing.T) {
102104
}
103105

104106
var buf bytes.Buffer
105-
printListAsJSON(&buf, crontabs)
107+
if err := printListAsJSON(&buf, crontabs); err != nil {
108+
t.Fatalf("printListAsJSON returned error: %v", err)
109+
}
106110

107111
var parsed []json.RawMessage
108112
if err := json.Unmarshal(buf.Bytes(), &parsed); err != nil {
@@ -137,7 +141,9 @@ func TestPrintListAsJSONStructure(t *testing.T) {
137141

138142
func TestPrintListAsJSONEmptyCrontabs(t *testing.T) {
139143
var buf bytes.Buffer
140-
printListAsJSON(&buf, []*lib.Crontab{})
144+
if err := printListAsJSON(&buf, []*lib.Crontab{}); err != nil {
145+
t.Fatalf("printListAsJSON returned error: %v", err)
146+
}
141147

142148
var parsed []json.RawMessage
143149
if err := json.Unmarshal(buf.Bytes(), &parsed); err != nil {
@@ -150,7 +156,9 @@ func TestPrintListAsJSONEmptyCrontabs(t *testing.T) {
150156

151157
func TestPrintListAsJSONNilCrontabs(t *testing.T) {
152158
var buf bytes.Buffer
153-
printListAsJSON(&buf, nil)
159+
if err := printListAsJSON(&buf, nil); err != nil {
160+
t.Fatalf("printListAsJSON returned error: %v", err)
161+
}
154162

155163
var parsed []json.RawMessage
156164
if err := json.Unmarshal(buf.Bytes(), &parsed); err != nil {
@@ -175,7 +183,9 @@ func TestPrintListAsJSONSkipsEmptyCrontabs(t *testing.T) {
175183
}
176184

177185
var buf bytes.Buffer
178-
printListAsJSON(&buf, crontabs)
186+
if err := printListAsJSON(&buf, crontabs); err != nil {
187+
t.Fatalf("printListAsJSON returned error: %v", err)
188+
}
179189

180190
var parsed []json.RawMessage
181191
if err := json.Unmarshal(buf.Bytes(), &parsed); err != nil {
@@ -186,6 +196,38 @@ func TestPrintListAsJSONSkipsEmptyCrontabs(t *testing.T) {
186196
}
187197
}
188198

199+
func TestPrintListAsJSONExcludesNonJobLines(t *testing.T) {
200+
crontabs := []*lib.Crontab{
201+
makeCrontab("/etc/crontab", false, []*lib.Line{
202+
makeLine("0 * * * *", "/usr/bin/backup"),
203+
// Comment line (no command)
204+
{IsComment: true, FullLine: "# this is a comment", LineNumber: 2},
205+
// Env var line (no command)
206+
{FullLine: "SHELL=/bin/bash", LineNumber: 3},
207+
makeLine("30 2 * * *", "/usr/bin/cleanup"),
208+
}),
209+
}
210+
211+
var buf bytes.Buffer
212+
if err := printListAsJSON(&buf, crontabs); err != nil {
213+
t.Fatalf("printListAsJSON returned error: %v", err)
214+
}
215+
216+
var parsed []struct {
217+
Lines []json.RawMessage `json:"lines"`
218+
}
219+
if err := json.Unmarshal(buf.Bytes(), &parsed); err != nil {
220+
t.Fatalf("failed to parse JSON: %v", err)
221+
}
222+
223+
if len(parsed) != 1 {
224+
t.Fatalf("expected 1 crontab, got %d", len(parsed))
225+
}
226+
if len(parsed[0].Lines) != 2 {
227+
t.Errorf("expected 2 job lines (excluding comment and env var), got %d", len(parsed[0].Lines))
228+
}
229+
}
230+
189231
func TestPrintListAsJSONContainsCommandText(t *testing.T) {
190232
crontabs := []*lib.Crontab{
191233
makeCrontab("/etc/crontab", false, []*lib.Line{
@@ -194,7 +236,9 @@ func TestPrintListAsJSONContainsCommandText(t *testing.T) {
194236
}
195237

196238
var buf bytes.Buffer
197-
printListAsJSON(&buf, crontabs)
239+
if err := printListAsJSON(&buf, crontabs); err != nil {
240+
t.Fatalf("printListAsJSON returned error: %v", err)
241+
}
198242

199243
if !bytes.Contains(buf.Bytes(), []byte("/usr/bin/test arg1 arg2")) {
200244
t.Errorf("expected command in JSON output, got: %s", buf.String())

0 commit comments

Comments
 (0)