diff --git a/tools/apidiff/delta/delta.go b/tools/apidiff/delta/delta.go index fc2830796b59..e23e4426a693 100644 --- a/tools/apidiff/delta/delta.go +++ b/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 } diff --git a/tools/apidiff/delta/delta_test.go b/tools/apidiff/delta/delta_test.go index d5e1bb56f742..48ca016d5277 100644 --- a/tools/apidiff/delta/delta_test.go +++ b/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"}, }, } diff --git a/tools/apidiff/exports/exports.go b/tools/apidiff/exports/exports.go index 2efcd6d890b6..41f8faf71eba 100644 --- a/tools/apidiff/exports/exports.go +++ b/tools/apidiff/exports/exports.go @@ -51,11 +51,47 @@ 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. @@ -63,7 +99,7 @@ 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 } diff --git a/tools/apidiff/exports/package.go b/tools/apidiff/exports/package.go index de4b566ac73b..2a20064141ea 100644 --- a/tools/apidiff/exports/package.go +++ b/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 } diff --git a/tools/apidiff/exports/package_test.go b/tools/apidiff/exports/package_test.go index 3039ff32fd34..ae359a9ccf22 100644 --- a/tools/apidiff/exports/package_test.go +++ b/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"}, }, }, }}, diff --git a/tools/apidiff/markdown/markdown_test.go b/tools/apidiff/markdown/markdown_test.go index 3e86ef676e90..cdc2f69a66a4 100644 --- a/tools/apidiff/markdown/markdown_test.go +++ b/tools/apidiff/markdown/markdown_test.go @@ -1,3 +1,17 @@ +// Copyright 2018 Microsoft Corporation +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package markdown import "testing" diff --git a/tools/apidiff/report/report.go b/tools/apidiff/report/report.go index de75a209cf6e..5bab2f241e98 100644 --- a/tools/apidiff/report/report.go +++ b/tools/apidiff/report/report.go @@ -16,12 +16,10 @@ package report import ( "fmt" - "sort" - "strings" - "github.com/Azure/azure-sdk-for-go/tools/apidiff/delta" "github.com/Azure/azure-sdk-for-go/tools/apidiff/exports" "github.com/Azure/azure-sdk-for-go/tools/apidiff/markdown" + "sort" ) // Package represents a per-package report that contains additive and breaking changes. @@ -219,16 +217,10 @@ func writeFuncs(funcs map[string]exports.Func, subheader string, md *markdown.Wr items := make([]string, len(funcs)) i := 0 for k, v := range funcs { - params := "" - if v.Params != nil { - params = *v.Params - } - returns := "" - if v.Returns != nil { - returns = *v.Returns - if strings.Index(returns, ",") > -1 { - returns = fmt.Sprintf("(%s)", returns) - } + params := v.Params.String() + returns := v.Returns.String() + if len(v.Returns) > 1 { + returns = fmt.Sprintf("(%s)", v.Returns.String()) } items[i] = fmt.Sprintf("1. %s(%s) %s", k, params, returns) i++