Fixing bug where comment added to multiline attribute was added as suffix (#1434)
- Also implementing tests for cmdComment
Co-authored-by: Tim Malmström <oreflow@google.com>
diff --git a/build/BUILD.bazel b/build/BUILD.bazel
index 2b7fcf0..e1d3f6f 100644
--- a/build/BUILD.bazel
+++ b/build/BUILD.bazel
@@ -39,6 +39,7 @@
"quote_test.go",
"rewrite_test.go",
"rule_test.go",
+ "utils_test.go",
"walk_test.go",
],
data = glob(["testdata/*"]) + [
diff --git a/build/utils.go b/build/utils.go
index 1e7c2b9..c0b2bd3 100644
--- a/build/utils.go
+++ b/build/utils.go
@@ -79,3 +79,9 @@
return []string{}
}
}
+
+// IsMultiLine returns whether an Expr is multiline or not.
+func IsMultiLine(param Expr) bool {
+ start, end := param.Span()
+ return start.Line != end.Line
+}
diff --git a/build/utils_test.go b/build/utils_test.go
new file mode 100644
index 0000000..8edba18
--- /dev/null
+++ b/build/utils_test.go
@@ -0,0 +1,122 @@
+/*
+Copyright 2025 Google LLC
+
+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
+
+ https://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 build
+
+import (
+ "testing"
+)
+
+func TestIsMultiline(t *testing.T) {
+ var tests = []struct {
+ name string
+ buildFile string
+ exprToCheck func(Expr) bool
+ wantIsMultiline bool
+ }{
+ {
+ name: "single_line_call",
+ exprToCheck: func(expr Expr) bool {
+ if call, ok := expr.(*CallExpr); ok {
+ if calledIdent, ok := call.X.(*Ident); ok && calledIdent.Name == "foo" {
+ return true
+ }
+ }
+ return false
+ },
+ buildFile: `
+ foo(name = "bar")`,
+ wantIsMultiline: false,
+ },
+ {
+ name: "multiline_call",
+ exprToCheck: func(expr Expr) bool {
+ if call, ok := expr.(*CallExpr); ok {
+ if calledIdent, ok := call.X.(*Ident); ok && calledIdent.Name == "foo" {
+ return true
+ }
+ }
+ return false
+ },
+ buildFile: `
+ foo(
+ name = "bar",
+ )`,
+ wantIsMultiline: true,
+ },
+ {
+ name: "single_line_attribute",
+ exprToCheck: func(expr Expr) bool {
+ if assign, ok := expr.(*AssignExpr); ok {
+ if calledIdent, ok := assign.LHS.(*Ident); ok && calledIdent.Name == "foo_attr" {
+ return true
+ }
+ }
+ return false
+ },
+ buildFile: `
+ foo(
+ name = "bar",
+ foo_attr = "on_a_single_line",
+ )`,
+ wantIsMultiline: false,
+ },
+ {
+ name: "multi_line_attribute",
+ exprToCheck: func(expr Expr) bool {
+ if assign, ok := expr.(*AssignExpr); ok {
+ if calledIdent, ok := assign.LHS.(*Ident); ok && calledIdent.Name == "foo_attr" {
+ return true
+ }
+ }
+ return false
+ },
+ buildFile: `
+ foo(
+ name = "bar",
+ foo_attr = [
+ "attribute",
+ "which spans",
+ "multiple lines",
+ ],
+ )`,
+ wantIsMultiline: true,
+ },
+ }
+
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ bld, err := Parse("BUILD", []byte(tc.buildFile))
+ if err != nil {
+ t.Fatal(err)
+ }
+ var testedExpr *Expr
+ Walk(bld, func(x Expr, _ []Expr) {
+ if tc.exprToCheck(x) {
+ testedExpr = &x
+ }
+ })
+ if testedExpr == nil {
+ t.Fatal("Unable to find expression to test")
+ }
+
+ got := IsMultiLine(*testedExpr)
+ if got != tc.wantIsMultiline {
+ t.Fatalf("IsMultiline returned incorrect value, got: %t, expected: %t", got, tc.wantIsMultiline)
+ }
+ })
+ }
+}
diff --git a/edit/buildozer.go b/edit/buildozer.go
index 5527d65..d6eb110 100644
--- a/edit/buildozer.go
+++ b/edit/buildozer.go
@@ -117,7 +117,7 @@
env.Rule.Call.Comments.Before = comment
case 2: // Attach to an attribute
if attr := env.Rule.AttrDefn(env.Args[0]); attr != nil {
- if fullLine {
+ if fullLine || build.IsMultiLine(attr) {
attr.LHS.Comment().Before = comment
} else {
attr.RHS.Comment().Suffix = comment
diff --git a/edit/buildozer_test.go b/edit/buildozer_test.go
index c1814a4..f80a4a0 100644
--- a/edit/buildozer_test.go
+++ b/edit/buildozer_test.go
@@ -123,6 +123,124 @@
}
}
+func TestCmdComment(t *testing.T) {
+ var tests = []struct {
+ name string
+ args []string
+ buildFile string
+ want string
+ }{
+ {
+ name: "adds_comment_to_rule_call",
+ args: []string{"New Comment"},
+ buildFile: `foo(
+ name = "foo",
+)`,
+ want: `# New Comment
+foo(
+ name = "foo",
+)`,
+ }, {
+ name: "adds_comment_to_single_line_attribute",
+ args: []string{"deps", "New Comment"},
+ buildFile: `foo(
+ name = "foo",
+ deps = ["//some/dep"],
+)`,
+ want: `foo(
+ name = "foo",
+ deps = ["//some/dep"], # New Comment
+)`,
+ }, {
+ name: "adds_multiline_comment_to_single_line_attribute",
+ args: []string{"deps", "New Comment\nWith Multiple Lines"},
+ buildFile: `foo(
+ name = "foo",
+ deps = ["//some/dep"],
+)`,
+ want: `foo(
+ name = "foo",
+ # New Comment
+ # With Multiple Lines
+ deps = ["//some/dep"],
+)`,
+ }, {
+ name: "adds_comment_to_multiline_attribute",
+ args: []string{"deps", "New Comment"},
+ buildFile: `foo(
+ name = "foo",
+ deps = [
+ "//some/dep",
+ "//some/other/dep",
+ ],
+)`,
+ want: `foo(
+ name = "foo",
+ # New Comment
+ deps = [
+ "//some/dep",
+ "//some/other/dep",
+ ],
+)`,
+ }, {
+ name: "adds_comment_to_element_in_list",
+ args: []string{"deps", "//some/other/dep", "New Comment"},
+ buildFile: `foo(
+ name = "foo",
+ deps = [
+ "//some/dep",
+ "//some/other/dep",
+ ],
+)`,
+ want: `foo(
+ name = "foo",
+ deps = [
+ "//some/dep",
+ "//some/other/dep", # New Comment
+ ],
+)`,
+ }, {
+ name: "adds_multiline_comment_to_element_in_list",
+ args: []string{"deps", "//some/other/dep", "New Comment\nWith Multiple Lines"},
+ buildFile: `foo(
+ name = "foo",
+ deps = [
+ "//some/dep",
+ "//some/other/dep",
+ ],
+)`,
+ want: `foo(
+ name = "foo",
+ deps = [
+ "//some/dep",
+ # New Comment
+ # With Multiple Lines
+ "//some/other/dep",
+ ],
+)`,
+ },
+ }
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ bld, err := build.Parse("BUILD", []byte(tc.buildFile))
+ if err != nil {
+ t.Error(err)
+ }
+ rl := bld.Rules("foo")[0]
+ env := CmdEnvironment{
+ File: bld,
+ Rule: rl,
+ Args: tc.args,
+ }
+ bld, _ = cmdComment(NewOpts(), env)
+ got := strings.TrimSpace(string(build.Format(bld)))
+ if diff := cmp.Diff(got, tc.want); diff != "" {
+ t.Fatalf("cmdComment returned unexpected diff %s", diff)
+ }
+ })
+ }
+}
+
type targetExpressionToBuildFilesTestCase struct {
rootDir, target string
buildFiles []string
