Respond to reviewer comments

gazelle/BUILD.bazel

@@ -1,4 +1,4 @@
-# Copyright 2018 The Bazel Authors. All rights reserved.
+# Copyright 2019 The Bazel Authors. All rights reserved.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -54,7 +54,10 @@ go_test(
     embed = [":go_default_library"],
     importpath = "github.com/bazelbuild/rules_sass/gazelle",
     rundir = ".",
-    deps = ["@bazel_gazelle//testtools:go_default_library"],
+    deps = [
+        "@bazel_gazelle//testtools:go_default_library",
+        "@io_bazel_rules_go//go/tools/bazel:go_default_library",
+    ],
 )
 
 gazelle_binary(

gazelle/config.go

@@ -1,4 +1,4 @@
-/* Copyright 2018 The Bazel Authors. All rights reserved.
+/* Copyright 2019 The Bazel Authors. All rights reserved.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.

gazelle/fileinfo.go

@@ -1,4 +1,4 @@
-/* Copyright 2018 The Bazel Authors. All rights reserved.
+/* Copyright 2019 The Bazel Authors. All rights reserved.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -17,6 +17,7 @@ package gazelle
 
 import (
 	"bytes"
+	"fmt"
 	"log"
 	"os"
 	"path/filepath"
@@ -50,53 +51,80 @@ func sassFileInfo(dir, name string) FileInfo {
 
 	s := parser.New(file)
 
-	for t := s.Scan(); t.Type() != "EOF"; t = s.Scan() {
-		importString := ""
+	// Abstract the scan so that it doesn't return whitespace.
+	scan := func() parser.Token {
+		for {
+			t := s.Scan()
+			if _, ok := t.(*parser.WhiteSpace); !ok {
+				return t
+			}
+		}
+	}
+
+	// parseError is a convenience wrapper to print to stderr a parse error message
+	// in a standardized format.
+	parseError := func(msg string, args ...interface{}) {
+		// Prepend the file.Name() so we tell the user which file had the parse error.
+		args = append([]interface{}{file.Name()}, args...)
+		fmt.Fprintf(os.Stderr, "%s: "+msg+"\n", args...)
+	}
+
+	for t := scan(); t.Type() != "EOF"; t = scan() {
+		var imports []string
 		switch v := t.(type) {
 		case *parser.At:
-			if i, ok := v.Ident.(*parser.Ident); ok {
-				if i.Value == "import" {
-					for {
-						// Consume all whitespace.
-						t = s.Scan()
-						if _, ok := t.(*parser.WhiteSpace); !ok {
-							break
-						}
+			if i, ok := v.Ident.(*parser.Ident); ok && i.Value == "import" {
+				// import_stmt ::= <At <Ident "import">> <String> (<Comma> <String>)+ <Semicolon>
+				// Scan the next symbol. It should be either a <String>
+				t = scan()
+				if s, ok := t.(*parser.String); ok {
+					imports = append(imports, s.Value)
+				} else {
+					parseError("expected a string but got a %v\n", t.Type())
+					continue
+				}
+
+				// Scan the next symbol. It should be either a <Comma> or a <Semicolon>
+				t = scan()
+
+				// Loop through the next tokens in case the user provided a comma
+				// separated list of files to include.
+				for {
+					if _, ok := t.(*parser.Comma); !ok {
+						break
 					}
 
+					t = scan()
 					if s, ok := t.(*parser.String); ok {
-						importString = s.Value
-					}
-				}
-			}
-		case *parser.Ident:
-			if v.Value == "import" {
-				for {
-					// Consume all whitespace.
-					t = s.Scan()
-					if _, ok := t.(*parser.WhiteSpace); !ok {
+						imports = append(imports, s.Value)
+					} else {
 						break
 					}
+
+					t = scan()
 				}
 
-				if s, ok := t.(*parser.String); ok {
-					importString = s.Value
+				if _, ok := t.(*parser.Semicolon); !ok {
+					// If the last character isn't a semicolon then it isn't a valid
+					// import statement. Don't add it to the list of imports.
+					parseError("imports should end in semicolons\n")
+					continue
+				}
+				if len(imports) > 0 {
+					info.Imports = append(info.Imports, imports...)
 				}
 			}
 		}
-		if importString != "" {
-			info.Imports = append(info.Imports, importString)
-		}
 	}
 
 	sort.Strings(info.Imports)
 
 	return info
 }
 
-// unquoteSASSString takes a string that has a complex quoting around it
+// unquoteSassString takes a string that has a complex quoting around it
 // and returns a string without the complex quoting.
-func unquoteSASSString(q []byte) string {
+func unquoteSassString(q []byte) string {
 	// Adjust quotes so that Unquote is happy. We need a double quoted string
 	// without unescaped double quote characters inside.
 	noQuotes := bytes.Split(q[1:len(q)-1], []byte{'"'})

gazelle/fileinfo_test.go

@@ -1,4 +1,4 @@
-/* Copyright 2018 The Bazel Authors. All rights reserved.
+/* Copyright 2019 The Bazel Authors. All rights reserved.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -36,29 +36,29 @@ func TestProtoFileInfo(t *testing.T) {
 		}, {
 			desc: "import single quote",
 			name: "single.sass",
-			sass: `import 'single';`,
+			sass: `@import 'single';`,
 			want: FileInfo{
 				Imports: []string{"single"},
 			},
 		}, {
 			desc: "import double quote",
 			name: "double.sass",
-			sass: `import "double";`,
+			sass: `@import "double";`,
 			want: FileInfo{
 				Imports: []string{"double"},
 			},
 		}, {
 			desc: "import two",
 			name: "two.sass",
-			sass: `import "first";
-import "second";`,
+			sass: `@import "first";
+@import "second";`,
 			want: FileInfo{
 				Imports: []string{"first", "second"},
 			},
 		}, {
 			desc: "import depth",
 			name: "deep.sass",
-			sass: `import "from/internal/package";`,
+			sass: `@import "from/internal/package";`,
 			want: FileInfo{
 				Imports: []string{"from/internal/package"},
 			},

gazelle/gazellebinary_test.go

@@ -1,4 +1,4 @@
-/* Copyright 2018 The Bazel Authors. All rights reserved.
+/* Copyright 2019 The Bazel Authors. All rights reserved.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@ import (
 	"testing"
 
 	"github.com/bazelbuild/bazel-gazelle/testtools"
+	"github.com/bazelbuild/rules_go/go/tools/bazel"
 )
 
 var (
@@ -64,7 +65,11 @@ func TestMain(m *testing.M) {
 }
 
 func TestTestdata(t *testing.T) {
-	testDataDir := filepath.Join("gazelle", "testdata")
+	testDataDir, err := bazel.Runfile(filepath.Join("gazelle", "testdata"))
+	if err != nil {
+		t.Fatalf("Error finding runfile gazelle/testdata: %s", err)
+	}
+
 	testDataFiles, err := ioutil.ReadDir(testDataDir)
 	if err != nil {
 		t.Fatalf("Error enumerating test modes: %s", err)
@@ -86,8 +91,16 @@ func TestTestdata(t *testing.T) {
 		t.Run(testSuite, func(t *testing.T) {
 			var files []testtools.FileSpec
 			var want []testtools.FileSpec
-			// Walk testSuite.Name() to find all the input files and add them to the files or the expectations
+			// Walk testSuite.Name() to find all the input files and add them to the
+			// files or the expectations
 			testSuitePath := filepath.Join(testDataDir, testSuite)
+
+			// If the suite contains a file named "skip" then skip the suite.
+			_, err = os.Stat(filepath.Join(testSuitePath, "skip"))
+			if !os.IsNotExist(err) {
+				t.Skip()
+			}
+
 			err := filepath.Walk(testSuitePath, func(path string, info os.FileInfo, err error) error {
 				// There is no need to process directories. Skip them.
 				if info.IsDir() {
@@ -128,10 +141,6 @@ func TestTestdata(t *testing.T) {
 			dir, cleanup := testtools.CreateFiles(t, files)
 			defer cleanup()
 
-			t.Logf("Dir: %q", dir)
-			t.Logf("Files: %v", files)
-			t.Logf("Want: %v", want)
-
 			if err := runGazelle(dir); err != nil {
 				t.Fatal(err)
 			}

gazelle/resolver.go

@@ -1,4 +1,4 @@
-/* Copyright 2018 The Bazel Authors. All rights reserved.
+/* Copyright 2019 The Bazel Authors. All rights reserved.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -67,7 +67,7 @@ func (s *sasslang) Imports(c *config.Config, r *rule.Rule, f *rule.File) []resol
 // the embedding rule will be indexed. The embedding rule will inherit
 // the imports of the embedded rule.
 func (s *sasslang) Embeds(r *rule.Rule, from label.Label) []label.Label {
-	// SASS doesn't have a concept of embedding as far as I know.
+	// Sass doesn't have a concept of embedding as far as I know.
 	return nil
 }
 

gazelle/sass.go

@@ -1,4 +1,4 @@
-/* Copyright 2018 The Bazel Authors. All rights reserved.
+/* Copyright 2019 The Bazel Authors. All rights reserved.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -111,22 +111,22 @@ func (s *sasslang) GenerateRules(args language.GenerateArgs) language.GenerateRe
 	for _, file := range files {
 		f := file.Name()
 
-		// Only generate SASS entries for sass files (.scss/.sass)
+		// Only generate Sass entries for sass files (.scss/.sass)
 		if !strings.HasSuffix(f, ".sass") && !strings.HasSuffix(f, ".scss") {
 			continue
 		}
 
 		fileInfo := sassFileInfo(args.Dir, f)
 		imports = append(imports, fileInfo.Imports)
 
-		// The primary entrypoint on SASS is a main.scss file.
+		// The primary entrypoint on Sass is a main.scss file.
 		if f == "main.scss" {
 			rule := rule.NewRule("sass_binary", base)
 
 			rule.SetAttr("src", "main.scss")
 			rules = append(rules, rule)
 		} else if strings.HasPrefix(filepath.Base(f), "_") {
-			// Libraries in SASS have filenames that start with _.
+			// Libraries in Sass have filenames that start with _.
 			base = filepath.Base(f)
 
 			rule := rule.NewRule("sass_library", base[1:len(base)-5])

gazelle/testdata/.gitignore

@@ -0,0 +1 @@
+**/skip

gazelle/testdata/BUILD.bazel

@@ -2,6 +2,6 @@
 
 filegroup(
     name = "testdata",
-    srcs = glob(["testdata/**"]),
+    srcs = glob(["**"]),
     visibility = ["//gazelle:__pkg__"],
 )

gazelle/testdata/README.md

@@ -15,3 +15,11 @@ documentation](https://docs.bazel.build/versions/master/skylark/build-style.html
 "Recursive globs make BUILD files difficult to reason about because they skip
 subdirectories containing BUILD files."
 
+## Skipping tests
+
+If you are only interested in excluding a test suite from the runnable set, you
+can place a `skip` file in the root of that suite and it will be excluded. If
+you wanted to skip the `simple` test case, you would would create a file at
+`gazelle/testdata/simple/skip`. This can be dome with the `touch` command.
+
+NOTE: `skip` files are ignored by the `.gitignore` file.

gazelle/testdata/comments/hello_world/main.scss

@@ -1,8 +1,11 @@
-/* Comment at the top of the file */
+/* Comment at the top of the file
+*/
 @import "shared/fonts";
 /* Comment between the imports */
 @import "shared/colors";
-/* Comment after the imports */
+/* Comment after the imports
+@import "shared/nonexistant";
+*/
 
 html {
   body {

gazelle/testdata/simple/repeating/BUILD.bazel.out

@@ -0,0 +1,10 @@
+load("@io_bazel_rules_sass//:defs.bzl", "sass_binary")
+
+sass_binary(
+    name = "repeating",
+    src = "main.scss",
+    deps = [
+        "//shared:colors",
+        "//shared:fonts",
+    ],
+)

gazelle/testdata/simple/repeating/main.scss

@@ -0,0 +1,12 @@
+/* Multiple import statements on one line. */
+@import "shared/fonts", "shared/colors";
+
+html {
+  body {
+    font-family: $default-font-stack;
+    h1 {
+      font-family: $modern-font-stack;
+      color: $example-red;
+    }
+  }
+}