Skip to content

Commit 819935a

Browse files
bahrmichaelbahrmichael
committed
review updates
1 parent e63b5f4 commit 819935a

4 files changed

Lines changed: 77 additions & 107 deletions

File tree

‎cmd/src/abc_variables_delete.go‎

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -42,30 +42,19 @@ Examples:
4242

4343
instanceID := cmd.Args().First()
4444
client := cfg.apiClient(clicompat.APIFlagsFromCmd(cmd), cmd.Writer)
45-
varArgs := abcVariableArgs(cmd.StringSlice("var"))
45+
variableNames := append(cmd.Args().Tail(), cmd.StringSlice("var")...)
4646

47-
if len(varArgs) == 0 {
48-
return cmderrors.Usage("must provide at least one variable name")
49-
}
50-
return runABCVariablesDelete(ctx, client, instanceID, cmd.Args().Tail(), varArgs, cmd.Writer, cmd.Bool("get-curl"))
47+
return runABCVariablesDelete(ctx, client, instanceID, variableNames, cmd.Writer, cmd.Bool("get-curl"))
5148
},
5249
})
5350

54-
func parseABCVariableNames(positional []string, flagged abcVariableArgs) ([]string, error) {
55-
variableNames := append([]string{}, positional...)
56-
variableNames = append(variableNames, flagged...)
57-
58-
if slices.Contains(variableNames, "") {
59-
return nil, cmderrors.Usage("variable names must not be empty")
51+
func runABCVariablesDelete(ctx context.Context, client api.Client, instanceID string, variableNames []string, output io.Writer, getCurl bool) error {
52+
if len(variableNames) == 0 {
53+
return cmderrors.Usage("must provide at least one variable name")
6054
}
6155

62-
return variableNames, nil
63-
}
64-
65-
func runABCVariablesDelete(ctx context.Context, client api.Client, instanceID string, positional []string, flagged abcVariableArgs, output io.Writer, getCurl bool) error {
66-
variableNames, err := parseABCVariableNames(positional, flagged)
67-
if err != nil {
68-
return err
56+
if slices.Contains(variableNames, "") {
57+
return cmderrors.Usage("variable names must not be empty")
6958
}
7059

7160
variables := make([]map[string]string, 0, len(variableNames))
@@ -84,6 +73,6 @@ func runABCVariablesDelete(ctx context.Context, client api.Client, instanceID st
8473
return nil
8574
}
8675

87-
_, err = fmt.Fprintf(output, "Removed variables %q from workflow instance %q.\n", variableNames, instanceID)
76+
_, err := fmt.Fprintf(output, "Removed variables %q from workflow instance %q.\n", variableNames, instanceID)
8877
return err
8978
}

‎cmd/src/abc_variables_delete_test.go‎

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,45 @@
11
package main
22

33
import (
4+
"bytes"
5+
"context"
6+
"io"
47
"testing"
58

6-
"github.com/google/go-cmp/cmp"
9+
mockapi "github.com/sourcegraph/src-cli/internal/api/mock"
10+
"github.com/stretchr/testify/mock"
11+
"github.com/stretchr/testify/require"
712
)
813

9-
func TestParseABCVariableNames(t *testing.T) {
14+
func TestRunABCVariablesDelete(t *testing.T) {
1015
t.Parallel()
1116

12-
variableNames, err := parseABCVariableNames(
13-
[]string{"approval"},
14-
abcVariableArgs{"checkpoints", "prompt"},
15-
)
16-
if err != nil {
17-
t.Fatalf("parseABCVariableNames returned error: %v", err)
18-
}
19-
20-
if len(variableNames) != 3 {
21-
t.Fatalf("len(variableNames) = %d, want 3", len(variableNames))
22-
}
23-
want := []string{"approval", "checkpoints", "prompt"}
24-
if diff := cmp.Diff(want, variableNames); diff != "" {
25-
t.Fatalf("variableNames mismatch (-want +got):\n%s", diff)
26-
}
17+
client := new(mockapi.Client)
18+
request := &mockapi.Request{Response: `{"data":{"updateAgenticWorkflowInstanceVariables":{"id":"workflow"}}}`}
19+
output := &bytes.Buffer{}
20+
variableNames := []string{"approval", "checkpoints", "prompt"}
21+
22+
client.On("NewRequest", updateABCWorkflowInstanceVariablesMutation, map[string]any{
23+
"instanceID": "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==",
24+
"variables": []map[string]string{
25+
{"key": "approval", "value": "null"},
26+
{"key": "checkpoints", "value": "null"},
27+
{"key": "prompt", "value": "null"},
28+
},
29+
}).Return(request).Once()
30+
request.On("Do", context.Background(), mock.Anything).Return(true, nil).Once()
31+
32+
err := runABCVariablesDelete(context.Background(), client, "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==", variableNames, output, false)
33+
require.NoError(t, err)
34+
require.Equal(t, "Removed variables [\"approval\" \"checkpoints\" \"prompt\"] from workflow instance \"QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==\".\n", output.String())
35+
36+
client.AssertExpectations(t)
37+
request.AssertExpectations(t)
38+
}
39+
40+
func TestRunABCVariablesDeleteRejectsEmptyVariableName(t *testing.T) {
41+
t.Parallel()
42+
43+
err := runABCVariablesDelete(context.Background(), nil, "QWdlbnRpY1dvcmtmbG93SW5zdGFuY2U6MQ==", []string{"approval", ""}, io.Discard, false)
44+
require.ErrorContains(t, err, "variable names must not be empty")
2745
}

‎cmd/src/abc_variables_set.go‎

Lines changed: 25 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -59,73 +59,47 @@ NOTE: Values are interpreted as JSON literals when valid. Otherwise they are sen
5959

6060
instanceID := cmd.Args().First()
6161
client := cfg.apiClient(clicompat.APIFlagsFromCmd(cmd), cmd.Writer)
62-
return runABCVariablesSet(ctx, client, instanceID, cmd.Args().Tail(), abcVariableArgs(cmd.StringSlice("var")), cmd.Writer, cmd.Bool("get-curl"))
62+
abcVariables, err := parseABCVariables(cmd.Args().Tail(), cmd.StringSlice("var"))
63+
if err != nil {
64+
return err
65+
}
66+
return runABCVariablesSet(ctx, client, instanceID, abcVariables, cmd.Writer, cmd.Bool("get-curl"))
6367
},
6468
})
6569

66-
type abcVariableArgs []string
67-
68-
func (a *abcVariableArgs) String() string {
69-
return strings.Join(*a, ",")
70-
}
71-
72-
func (a *abcVariableArgs) Set(value string) error {
73-
*a = append(*a, value)
74-
return nil
75-
}
76-
77-
type abcVariable struct {
78-
Key string
79-
Value string
80-
}
81-
82-
func parseABCVariables(positional []string, flagged abcVariableArgs) ([]abcVariable, error) {
83-
rawVariables := append([]string{}, positional...)
84-
rawVariables = append(rawVariables, flagged...)
70+
func parseABCVariables(positional []string, flagged []string) (map[string]string, error) {
71+
rawVariables := append(positional, flagged...)
8572
if len(rawVariables) == 0 {
8673
return nil, cmderrors.Usage("must provide at least one variable assignment")
8774
}
8875

89-
variables := make([]abcVariable, 0, len(rawVariables))
90-
for _, rawVariable := range rawVariables {
91-
variable, err := parseABCVariable(rawVariable)
76+
variables := make(map[string]string, len(rawVariables))
77+
for _, v := range rawVariables {
78+
name, rawValue, ok := strings.Cut(v, "=")
79+
if !ok || name == "" {
80+
return nil, cmderrors.Usagef("invalid variable assignment %q: must be in <name>=<value> form", v)
81+
}
82+
83+
value, remove, err := marshalABCVariableValue(rawValue)
9284
if err != nil {
9385
return nil, err
9486
}
95-
variables = append(variables, variable)
96-
}
97-
98-
return variables, nil
99-
}
87+
if remove {
88+
return nil, cmderrors.Usagef("invalid variable assignment %q: use 'src abc variables delete <workflow-instance-id> %s' to remove a variable", rawValue, name)
89+
}
10090

101-
func parseABCVariable(raw string) (abcVariable, error) {
102-
name, rawValue, ok := strings.Cut(raw, "=")
103-
if !ok || name == "" {
104-
return abcVariable{}, cmderrors.Usagef("invalid variable assignment %q: must be in <name>=<value> form", raw)
91+
variables[name] = value
10592
}
10693

107-
value, remove, err := marshalABCVariableValue(rawValue)
108-
if err != nil {
109-
return abcVariable{}, err
110-
}
111-
if remove {
112-
return abcVariable{}, cmderrors.Usagef("invalid variable assignment %q: use 'src abc variables delete <workflow-instance-id> %s' to remove a variable", raw, name)
113-
}
114-
115-
return abcVariable{Key: name, Value: value}, nil
94+
return variables, nil
11695
}
11796

118-
func runABCVariablesSet(ctx context.Context, client api.Client, instanceID string, positional []string, flagged abcVariableArgs, output io.Writer, getCurl bool) error {
119-
variables, err := parseABCVariables(positional, flagged)
120-
if err != nil {
121-
return err
122-
}
123-
97+
func runABCVariablesSet(ctx context.Context, client api.Client, instanceID string, variables map[string]string, output io.Writer, getCurl bool) error {
12498
graphqlVariables := make([]map[string]string, 0, len(variables))
125-
for _, variable := range variables {
99+
for k, v := range variables {
126100
graphqlVariables = append(graphqlVariables, map[string]string{
127-
"key": variable.Key,
128-
"value": variable.Value,
101+
"key": k,
102+
"value": v,
129103
})
130104
}
131105

@@ -137,12 +111,7 @@ func runABCVariablesSet(ctx context.Context, client api.Client, instanceID strin
137111
return nil
138112
}
139113

140-
if len(variables) == 1 {
141-
_, err = fmt.Fprintf(output, "Set variable %q on workflow instance %q.\n", variables[0].Key, instanceID)
142-
return err
143-
}
144-
145-
_, err = fmt.Fprintf(output, "Updated %d variables on workflow instance %q.\n", len(variables), instanceID)
114+
_, err := fmt.Fprintf(output, "Updated %d variables on workflow instance %q.\n", len(variables), instanceID)
146115
return err
147116
}
148117

‎cmd/src/abc_variables_set_test.go‎

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ package main
22

33
import (
44
"testing"
5+
6+
"github.com/google/go-cmp/cmp"
57
)
68

79
// If we were to do a json marshalling roundtrip, it may break large integer literals.
@@ -26,25 +28,17 @@ func TestParseABCVariables(t *testing.T) {
2628

2729
variables, err := parseABCVariables(
2830
[]string{"prompt=tighten the review criteria", `title="test"`},
29-
abcVariableArgs{"checkpoints=[1,2,3]"},
31+
[]string{"checkpoints=[1,2,3]"},
3032
)
3133
if err != nil {
3234
t.Fatalf("parseABCVariables returned error: %v", err)
3335
}
3436

35-
if len(variables) != 3 {
36-
t.Fatalf("len(variables) = %d, want 3", len(variables))
37-
}
38-
39-
if variables[0].Key != "prompt" || variables[0].Value != `"tighten the review criteria"` {
40-
t.Fatalf("variables[0] = %#v, want prompt string variable", variables[0])
41-
}
42-
43-
if variables[1].Key != "title" || variables[1].Value != `"test"` {
44-
t.Fatalf("variables[1] = %#v, want quoted test string", variables[1])
45-
}
46-
47-
if variables[2].Key != "checkpoints" || variables[2].Value != "[1,2,3]" {
48-
t.Fatalf("variables[2] = %#v, want compact JSON array", variables[2])
37+
if diff := cmp.Diff(variables, map[string]string{
38+
"prompt": "\"tighten the review criteria\"",
39+
"title": "\"test\"",
40+
"checkpoints": "[1,2,3]",
41+
}); diff != "" {
42+
t.Errorf("err: %v", diff)
4943
}
5044
}

0 commit comments

Comments
 (0)