Refine the apidiff tool - make reorder of the function parameters a breaking change

tools/apidiff/delta/delta.go

@@ -224,16 +224,20 @@ func GetFuncSigChanges(lhs, rhs exports.Content) map[string]FuncSig {
 			continue
 		}
 		sig := FuncSig{}
-		if !safeStrCmp(lhs.Funcs[rhsKey].Params, rhsValue.Params) {
+		fromParam := lhs.Funcs[rhsKey].Params.String()
+		toParam := rhsValue.Params.String()
+		if fromParam != toParam {
 			sig.Params = &Signature{
-				From: safeFuncSig(lhs.Funcs[rhsKey].Params),
-				To:   safeFuncSig(rhsValue.Params),
+				From: safeFuncSig(fromParam),
+				To:   safeFuncSig(toParam),
 			}
 		}
-		if !safeStrCmp(lhs.Funcs[rhsKey].Returns, rhsValue.Returns) {
+		fromReturn := lhs.Funcs[rhsKey].Returns.String()
+		toReturn := rhsValue.Returns.String()
+		if fromReturn != toReturn {
 			sig.Returns = &Signature{
-				From: safeFuncSig(lhs.Funcs[rhsKey].Returns),
-				To:   safeFuncSig(rhsValue.Returns),
+				From: safeFuncSig(fromReturn),
+				To:   safeFuncSig(toReturn),
 			}
 		}
 
@@ -266,16 +270,20 @@ func GetInterfaceMethodSigChanges(lhs, rhs exports.Content) map[string]Interface
 				continue
 			}
 			sig := FuncSig{}
-			if !safeStrCmp(lhs.Interfaces[rhsKey].Methods[rhsMethod].Params, rhsSig.Params) {
+			fromParam := lhs.Interfaces[rhsKey].Methods[rhsMethod].Params.String()
+			toParam := rhsSig.Params.String()
+			if fromParam != toParam {
 				sig.Params = &Signature{
-					From: safeFuncSig(lhs.Interfaces[rhsKey].Methods[rhsMethod].Params),
-					To:   safeFuncSig(rhsSig.Params),
+					From: safeFuncSig(fromParam),
+					To:   safeFuncSig(toParam),
 				}
 			}
-			if !safeStrCmp(lhs.Interfaces[rhsKey].Methods[rhsMethod].Returns, rhsSig.Returns) {
+			fromReturn := lhs.Interfaces[rhsKey].Methods[rhsMethod].Returns.String()
+			toReturn := rhsSig.Returns.String()
+			if fromReturn != toReturn {
 				sig.Returns = &Signature{
-					From: safeFuncSig(lhs.Interfaces[rhsKey].Methods[rhsMethod].Returns),
-					To:   safeFuncSig(rhsSig.Returns),
+					From: safeFuncSig(fromReturn),
+					To:   safeFuncSig(toReturn),
 				}
 			}
 
@@ -333,19 +341,9 @@ func GetStructFieldChanges(lhs, rhs exports.Content) map[string]StructDef {
 	return sfc
 }
 
-func safeFuncSig(s *string) string {
-	if s == nil {
+func safeFuncSig(s string) string {
+	if len(s) == 0 {
 		return None
 	}
-	return *s
-}
-
-func safeStrCmp(lhs, rhs *string) bool {
-	if lhs == nil && rhs == nil {
-		return true
-	}
-	if lhs == nil || rhs == nil {
-		return false
-	}
-	return *lhs == *rhs
+	return s
 }

tools/apidiff/delta/delta_test.go

@@ -72,12 +72,53 @@ func Test_GetAddedExports(t *testing.T) {
 	}
 
 	fAdded := map[string]exports.Func{
-		"DoNothing2":                 {},
-		"Client.ExportData":          {Params: strPtr("context.Context, string, string, ExportRDBParameters"), Returns: strPtr("ExportDataFuture, error")},
-		"Client.ExportDataPreparer":  {Params: strPtr("context.Context, string, string, ExportRDBParameters"), Returns: strPtr("*http.Request, error")},
-		"Client.ExportDataSender":    {Params: strPtr("*http.Request"), Returns: strPtr("ExportDataFuture, error")},
-		"Client.ExportDataResponder": {Params: strPtr("*http.Response"), Returns: strPtr("autorest.Response, error")},
-		"ExportDataFuture.Result":    {Params: strPtr("Client"), Returns: strPtr("autorest.Response, error")},
+		"DoNothing2": {},
+		"Client.ExportData": {
+			Params: exports.ParameterList{
+				exports.Parameter{Name: strPtr("ctx"), Type: "context.Context"},
+				exports.Parameter{Name: strPtr("resourceGroupName"), Type: "string"},
+				exports.Parameter{Name: strPtr("name"), Type: "string"},
+				exports.Parameter{Name: strPtr("parameters"), Type: "ExportRDBParameters"},
+			},
+			Returns: exports.ReturnList{
+				"ExportDataFuture", "error",
+			},
+		},
+		"Client.ExportDataPreparer": {
+			Params: exports.ParameterList{
+				exports.Parameter{Name: strPtr("ctx"), Type: "context.Context"},
+				exports.Parameter{Name: strPtr("resourceGroupName"), Type: "string"},
+				exports.Parameter{Name: strPtr("name"), Type: "string"},
+				exports.Parameter{Name: strPtr("parameters"), Type: "ExportRDBParameters"},
+			},
+			Returns: exports.ReturnList{
+				"*http.Request", "error",
+			},
+		},
+		"Client.ExportDataSender": {
+			Params: exports.ParameterList{
+				exports.Parameter{Name: strPtr("req"), Type: "*http.Request"},
+			},
+			Returns: exports.ReturnList{
+				"ExportDataFuture", "error",
+			},
+		},
+		"Client.ExportDataResponder": {
+			Params: exports.ParameterList{
+				exports.Parameter{Name: strPtr("resp"), Type: "*http.Response"},
+			},
+			Returns: exports.ReturnList{
+				"autorest.Response", "error",
+			},
+		},
+		"ExportDataFuture.Result": {
+			Params: exports.ParameterList{
+				exports.Parameter{Name: strPtr("client"), Type: "Client"},
+			},
+			Returns: exports.ReturnList{
+				"autorest.Response", "error",
+			},
+		},
 	}
 
 	for k, v := range fAdded {
@@ -101,11 +142,26 @@ func Test_GetAddedExports(t *testing.T) {
 
 	iAdded := map[string]exports.Interface{
 		"NewInterface": {Methods: map[string]exports.Func{
-			"One": {Params: strPtr("int")},
-			"Two": {Returns: strPtr("error")},
+			"One": {
+				Params: exports.ParameterList{
+					exports.Parameter{Type: "int"},
+				},
+			},
+			"Two": {
+				Returns: exports.ReturnList{
+					"error",
+				},
+			},
 		}},
 		"SomeInterface": {Methods: map[string]exports.Func{
-			"NewMethod": {Params: strPtr("string"), Returns: strPtr("bool, error")},
+			"NewMethod": {
+				Params: exports.ParameterList{
+					exports.Parameter{Type: "string"},
+				},
+				Returns: exports.ReturnList{
+					"bool", "error",
+				},
+			},
 		}},
 	}
 
@@ -199,7 +255,14 @@ func Test_GetAddedInterfaceMethods(t *testing.T) {
 	added := map[string]exports.Interface{
 		"SomeInterface": {
 			Methods: map[string]exports.Func{
-				"NewMethod": {Params: strPtr("string"), Returns: strPtr("bool, error")},
+				"NewMethod": {
+					Params: exports.ParameterList{
+						exports.Parameter{Type: "string"},
+					},
+					Returns: exports.ReturnList{
+						"bool", "error",
+					},
+				},
 			},
 		},
 	}
@@ -269,23 +332,23 @@ func Test_GetFuncSigChanges(t *testing.T) {
 
 	changed := map[string]delta.FuncSig{
 		"DoNothing": {
-			Params: &delta.Signature{From: delta.None, To: "string"},
+			Params: &delta.Signature{From: delta.None, To: "s string"},
 		},
 		"DoNothingWithParam": {
-			Params: &delta.Signature{From: "int", To: delta.None},
+			Params: &delta.Signature{From: "foo int", To: delta.None},
 		},
 		"Client.List": {
-			Params:  &delta.Signature{From: "context.Context", To: "context.Context, string"},
+			Params:  &delta.Signature{From: "ctx context.Context", To: "ctx context.Context, s string"},
 			Returns: &delta.Signature{From: "ListResultPage, error", To: "ListResult, error"},
 		},
 		"Client.ListPreparer": {
-			Params: &delta.Signature{From: "context.Context", To: "context.Context, string"},
+			Params: &delta.Signature{From: "ctx context.Context", To: "ctx context.Context, s string"},
 		},
 		"Client.Delete": {
-			Params: &delta.Signature{From: "context.Context, string, string", To: "context.Context, string"},
+			Params: &delta.Signature{From: "ctx context.Context, resourceGroupName string, name string", To: "ctx context.Context, resourceGroupName string"},
 		},
 		"Client.DeletePreparer": {
-			Params: &delta.Signature{From: "context.Context, string, string", To: "context.Context, string"},
+			Params: &delta.Signature{From: "ctx context.Context, resourceGroupName string, name string", To: "ctx context.Context, resourceGroupName string"},
 		},
 	}
 

tools/apidiff/exports/exports.go

@@ -51,19 +51,55 @@ type Const struct {
 
 // Func contains parameter and return types of a function/method.
 type Func struct {
-	// a comma-delimited list of the param types
-	Params *string `json:"params,omitempty"`
+	// Params is a list of the parameters
+	Params ParameterList `json:"params"`
 
-	// a comma-delimited list of the return types
-	Returns *string `json:"returns,omitempty"`
+	// Returns is a list of the return types
+	Returns ReturnList `json:"returns"`
+}
+
+// ParameterList represents a list of parameters
+type ParameterList []Parameter
+
+// String ...
+func (pl ParameterList) String() string {
+	pList := make([]string, len(pl))
+	for i := range pl {
+		pList[i] = pl[i].String()
+	}
+	return strings.Join(pList, ", ")
+}
+
+// Parameter is the parameter of a function
+type Parameter struct {
+	// Name is the name of the parameter
+	Name *string `json:"name,omitempty"`
+	// Type is the type of the parameter
+	Type string `json:"type"`
+}
+
+// String ...
+func (p Parameter) String() string {
+	if p.Name == nil {
+		return p.Type
+	}
+	return fmt.Sprintf("%s %s", *p.Name, p.Type)
+}
+
+// ReturnList represent the return types of a function
+type ReturnList []string
+
+// String ...
+func (rl ReturnList) String() string {
+	return strings.Join(rl, ", ")
 }
 
 // Interface contains the list of methods for an interface.
 type Interface struct {
 	// a list of embedded interfaces
 	AnonymousFields []string `json:"anon,omitempty"`
 
-	// key/value pairs of the methd names and their definitions
+	// key/value pairs of the method names and their definitions
 	Methods map[string]Func
 }
 

tools/apidiff/exports/package.go

@@ -169,31 +169,25 @@ func (pkg Package) translateFieldList(fl []*ast.Field, cb func(*string, string,
 
 // creates a Func object from the specified ast.FuncType
 func (pkg Package) buildFunc(ft *ast.FuncType) (f Func) {
-	// appends a to s, comma-delimited style, and returns s
-	appendString := func(s, a string) string {
-		if s != "" {
-			s += ", "
-		}
-		s += a
-		return s
-	}
-
 	// build the params type list
 	if ft.Params.List != nil {
-		p := ""
+		var pl ParameterList
 		pkg.translateFieldList(ft.Params.List, func(n *string, t string, f *ast.Field) {
-			p = appendString(p, t)
+			pl = append(pl, Parameter{
+				Name: n,
+				Type: t,
+			})
 		})
-		f.Params = &p
+		f.Params = pl
 	}
 
 	// build the return types list
 	if ft.Results != nil {
-		r := ""
+		var rl ReturnList
 		pkg.translateFieldList(ft.Results.List, func(n *string, t string, f *ast.Field) {
-			r = appendString(r, t)
+			rl = append(rl, t)
 		})
-		f.Returns = &r
+		f.Returns = rl
 	}
 	return
 }

tools/apidiff/exports/package_test.go

@@ -71,21 +71,41 @@ func Test_Funcs(t *testing.T) {
 		exports.Func
 	}{
 		{"DoNothing", exports.Func{}},
-		{"DoNothingWithParam", exports.Func{Params: strPtr("int"), Returns: nil}},
-		{"UserAgent", exports.Func{Params: nil, Returns: strPtr("string")}},
-		{"Client.Delete", exports.Func{Params: strPtr("context.Context, string, string"), Returns: strPtr("DeleteFuture, error")}},
-		{"Client.ListSender", exports.Func{Params: strPtr("*http.Request"), Returns: strPtr("*http.Response, error")}},
+		{"DoNothingWithParam", exports.Func{
+			Params: exports.ParameterList{
+				exports.Parameter{Name: strPtr("foo"), Type: "int"},
+			},
+			Returns: nil,
+		}},
+		{"UserAgent", exports.Func{
+			Params:  nil,
+			Returns: []string{"string"},
+		}},
+		{"Client.Delete", exports.Func{
+			Params: exports.ParameterList{
+				exports.Parameter{Name: strPtr("ctx"), Type: "context.Context"},
+				exports.Parameter{Name: strPtr("resourceGroupName"), Type: "string"},
+				exports.Parameter{Name: strPtr("name"), Type: "string"},
+			},
+			Returns: []string{"DeleteFuture", "error"},
+		}},
+		{"Client.ListSender", exports.Func{
+			Params: exports.ParameterList{
+				exports.Parameter{Name: strPtr("req"), Type: "*http.Request"},
+			},
+			Returns: []string{"*http.Response", "error"},
+		}},
 	}
 
 	for _, test := range tests {
 		t.Run(test.fn, func(t *testing.T) {
 			f := exp.Funcs[test.fn]
 			if !reflect.DeepEqual(f.Params, test.Params) {
-				t.Logf("mismatched params, %s != %s", safeStr(f.Params), safeStr(test.Params))
+				t.Logf("mismatched params, %s != %s", f.Params.String(), test.Params.String())
 				t.Fail()
 			}
 			if !reflect.DeepEqual(f.Returns, test.Returns) {
-				t.Logf("mismatched returns, %s != %s", safeStr(f.Returns), safeStr(test.Returns))
+				t.Logf("mismatched returns, %s != %s", f.Returns.String(), test.Returns.String())
 				t.Fail()
 			}
 		})
@@ -109,20 +129,27 @@ func Test_Interfaces(t *testing.T) {
 					Returns: nil,
 				},
 				"Two": {
-					Params:  strPtr("bool"),
+					Params: exports.ParameterList{
+						exports.Parameter{Type: "bool"},
+					},
 					Returns: nil,
 				},
 				"Three": {
 					Params:  nil,
-					Returns: strPtr("string"),
+					Returns: []string{"string"},
 				},
 				"Four": {
-					Params:  strPtr("int"),
-					Returns: strPtr("error"),
+					Params: exports.ParameterList{
+						exports.Parameter{Type: "int"},
+					},
+					Returns: []string{"error"},
 				},
 				"Five": {
-					Params:  strPtr("int, bool"),
-					Returns: strPtr("int, error"),
+					Params: exports.ParameterList{
+						exports.Parameter{Type: "int"},
+						exports.Parameter{Type: "bool"},
+					},
+					Returns: []string{"int", "error"},
 				},
 			},
 		}},