chore: address @wlynch final comments

google · Feb 28, 2022 · d483853 · d483853
1 parent b994e09
commit d483853
Show file tree

Hide file tree

Showing 7 changed files with 46 additions and 35 deletions.
diff --git a/csv.go b/csv.go
@@ -16,9 +16,8 @@ package main
 
 import (
 	"context"
-	"fmt"
+	"encoding/csv"
 	"os"
-	"strings"
 
 	"github.com/golang/glog"
 	"github.com/google/go-licenses/licenses"
@@ -43,6 +42,8 @@ func init() {
 }
 
 func csvMain(_ *cobra.Command, args []string) error {
+	writer := csv.NewWriter(os.Stdout)
+
 	classifier, err := licenses.NewClassifier(confidenceThreshold)
 	if err != nil {
 		return err
@@ -53,8 +54,8 @@ func csvMain(_ *cobra.Command, args []string) error {
 		return err
 	}
 	for _, lib := range libs {
-		licenseName := "Unknown"
 		licenseURL := "Unknown"
+		licenseName := "Unknown"
 		if lib.LicensePath != "" {
 			name, _, err := classifier.Identify(lib.LicensePath)
 			if err == nil {
@@ -69,16 +70,10 @@ func csvMain(_ *cobra.Command, args []string) error {
 				glog.Warningf("Error discovering license URL: %s", err)
 			}
 		}
-		// Using ", " to join words makes vscode/terminal recognize the
-		// correct license URL. Otherwise, if there's no space after
-		// comma, vscode interprets the URL as concatenated with the
-		// license name after it.
-		// Also, the extra spaces does not affect csv syntax much, we
-		// can still copy the csv text and paste into Excel / Google
-		// Sheets.
-		if _, err := fmt.Fprintln(os.Stdout, strings.Join([]string{lib.Name(), licenseURL, licenseName}, ", ")); err != nil {
+		if err := writer.Write([]string{lib.Name(), licenseURL, licenseName}); err != nil {
 			return err
 		}
 	}
-	return nil
+	writer.Flush()
+	return writer.Error()
 }
diff --git a/licenses/git.go b/licenses/git.go
@@ -46,6 +46,9 @@ type GitRepo struct {
 // FindGitRepo finds the Git repository that contains the specified filePath
 // by searching upwards through the directory tree for a ".git" directory.
 func FindGitRepo(filePath string) (*GitRepo, error) {
+	// TODO(Bobgy): the "/" is used just to fix the test. git.go is not
+	// currently used, but I plan to bring it back to detect version of the
+	// main module in following up PRs.
 	path, err := findUpwards(filepath.Dir(filePath), gitRegexp, "/", nil)
 	if err != nil {
 		return nil, err

diff --git a/licenses/library.go b/licenses/library.go
@@ -140,6 +140,19 @@ func Libraries(ctx context.Context, classifier Classifier, importPaths ...string
 				glog.Warningf("module %s does not have dir and it's not vendored, cannot discover the license URL. Report to go-licenses developer if you see this.", lib.module.Path)
 			} else {
 				// This is vendored. Handle this known special case.
+
+				// Extra note why we identify a vendored package like this.
+				//
+				// For a normal package:
+				// * if it's not in a module, lib.module == nil
+				// * if it's in a module, lib.module.Dir != ""
+				// Only vendored modules will have lib.module != nil && lib.module.Path != "" && lib.module.Dir == "" as far as I know.
+				// So the if condition above is already very strict for vendored packages.
+				// On top of it, we checked the lib.LicensePath contains a vendor folder in it.
+				// So it's rare to have a false positive for both conditions at the same time, although it may happen in theory.
+				//
+				// These assumptions may change in the future,
+				// so we need to keep this updated with go tooling changes.
 				parentModDir := splits[0]
 				var parentPkg *packages.Package
 				for _, rootPkg := range rootPkgs {

diff --git a/testdata/modules/cli02/licenses.csv b/testdata/modules/cli02/licenses.csv
@@ -1,18 +1,18 @@
-github.com/fsnotify/fsnotify, https://github.com/fsnotify/fsnotify/blob/v1.4.9/LICENSE, BSD-3-Clause
-github.com/google/go-licenses/testdata/modules/cli02, https://github.com/google/go-licenses/blob/HEAD/testdata/modules/cli02/LICENSE, Apache-2.0
-github.com/hashicorp/hcl, https://github.com/hashicorp/hcl/blob/v1.0.0/LICENSE, MPL-2.0
-github.com/magiconair/properties, https://github.com/magiconair/properties/blob/v1.8.5/LICENSE.md, BSD-2-Clause
-github.com/mitchellh/go-homedir, https://github.com/mitchellh/go-homedir/blob/v1.1.0/LICENSE, MIT
-github.com/mitchellh/mapstructure, https://github.com/mitchellh/mapstructure/blob/v1.4.1/LICENSE, MIT
-github.com/pelletier/go-toml, https://github.com/pelletier/go-toml/blob/v1.9.3/LICENSE, Apache-2.0
-github.com/spf13/afero, https://github.com/spf13/afero/blob/v1.6.0/LICENSE.txt, Apache-2.0
-github.com/spf13/cast, https://github.com/spf13/cast/blob/v1.3.1/LICENSE, MIT
-github.com/spf13/cobra, https://github.com/spf13/cobra/blob/v1.1.3/LICENSE.txt, Apache-2.0
-github.com/spf13/jwalterweatherman, https://github.com/spf13/jwalterweatherman/blob/v1.1.0/LICENSE, MIT
-github.com/spf13/pflag, https://github.com/spf13/pflag/blob/v1.0.5/LICENSE, BSD-3-Clause
-github.com/spf13/viper, https://github.com/spf13/viper/blob/v1.8.0/LICENSE, MIT
-github.com/subosito/gotenv, https://github.com/subosito/gotenv/blob/v1.2.0/LICENSE, MIT
-golang.org/x/sys, https://cs.opensource.google/go/x/sys/+/977fb726:LICENSE, BSD-3-Clause
-golang.org/x/text, https://cs.opensource.google/go/x/text/+/v0.3.5:LICENSE, BSD-3-Clause
-gopkg.in/ini.v1, https://github.com/go-ini/ini/blob/v1.62.0/LICENSE, Apache-2.0
-gopkg.in/yaml.v2, https://github.com/go-yaml/yaml/blob/v2.4.0/LICENSE, Apache-2.0
+github.com/fsnotify/fsnotify,https://github.com/fsnotify/fsnotify/blob/v1.4.9/LICENSE,BSD-3-Clause
+github.com/google/go-licenses/testdata/modules/cli02,https://github.com/google/go-licenses/blob/HEAD/testdata/modules/cli02/LICENSE,Apache-2.0
+github.com/hashicorp/hcl,https://github.com/hashicorp/hcl/blob/v1.0.0/LICENSE,MPL-2.0
+github.com/magiconair/properties,https://github.com/magiconair/properties/blob/v1.8.5/LICENSE.md,BSD-2-Clause
+github.com/mitchellh/go-homedir,https://github.com/mitchellh/go-homedir/blob/v1.1.0/LICENSE,MIT
+github.com/mitchellh/mapstructure,https://github.com/mitchellh/mapstructure/blob/v1.4.1/LICENSE,MIT
+github.com/pelletier/go-toml,https://github.com/pelletier/go-toml/blob/v1.9.3/LICENSE,Apache-2.0
+github.com/spf13/afero,https://github.com/spf13/afero/blob/v1.6.0/LICENSE.txt,Apache-2.0
+github.com/spf13/cast,https://github.com/spf13/cast/blob/v1.3.1/LICENSE,MIT
+github.com/spf13/cobra,https://github.com/spf13/cobra/blob/v1.1.3/LICENSE.txt,Apache-2.0
+github.com/spf13/jwalterweatherman,https://github.com/spf13/jwalterweatherman/blob/v1.1.0/LICENSE,MIT
+github.com/spf13/pflag,https://github.com/spf13/pflag/blob/v1.0.5/LICENSE,BSD-3-Clause
+github.com/spf13/viper,https://github.com/spf13/viper/blob/v1.8.0/LICENSE,MIT
+github.com/subosito/gotenv,https://github.com/subosito/gotenv/blob/v1.2.0/LICENSE,MIT
+golang.org/x/sys,https://cs.opensource.google/go/x/sys/+/977fb726:LICENSE,BSD-3-Clause
+golang.org/x/text,https://cs.opensource.google/go/x/text/+/v0.3.5:LICENSE,BSD-3-Clause
+gopkg.in/ini.v1,https://github.com/go-ini/ini/blob/v1.62.0/LICENSE,Apache-2.0
+gopkg.in/yaml.v2,https://github.com/go-yaml/yaml/blob/v2.4.0/LICENSE,Apache-2.0
diff --git a/testdata/modules/hello01/licenses.csv b/testdata/modules/hello01/licenses.csv
@@ -1 +1 @@
-github.com/google/go-licenses/testdata/modules/hello01, https://github.com/google/go-licenses/blob/HEAD/testdata/modules/hello01/LICENSE, Apache-2.0
+github.com/google/go-licenses/testdata/modules/hello01,https://github.com/google/go-licenses/blob/HEAD/testdata/modules/hello01/LICENSE,Apache-2.0
diff --git a/testdata/modules/replace04/licenses.csv b/testdata/modules/replace04/licenses.csv
@@ -1,2 +1,2 @@
-github.com/google/go-licenses/testdata/modules/replace04, https://github.com/google/go-licenses/blob/HEAD/testdata/modules/replace04/LICENSE, Apache-2.0
-github.com/mitchellh/go-homedir, https://github.com/mitchellh/go-homedir/blob/v1.0.0/LICENSE, MIT
+github.com/google/go-licenses/testdata/modules/replace04,https://github.com/google/go-licenses/blob/HEAD/testdata/modules/replace04/LICENSE,Apache-2.0
+github.com/mitchellh/go-homedir,https://github.com/mitchellh/go-homedir/blob/v1.0.0/LICENSE,MIT
diff --git a/testdata/modules/vendored03/licenses.csv b/testdata/modules/vendored03/licenses.csv
@@ -1,2 +1,2 @@
-github.com/google/go-licenses/testdata/modules/vendored03, https://github.com/google/go-licenses/blob/HEAD/testdata/modules/vendored03/LICENSE, Apache-2.0
-github.com/mitchellh/go-homedir, https://github.com/google/go-licenses/blob/HEAD/testdata/modules/vendored03/vendor/github.com/mitchellh/go-homedir/LICENSE, MIT
+github.com/google/go-licenses/testdata/modules/vendored03,https://github.com/google/go-licenses/blob/HEAD/testdata/modules/vendored03/LICENSE,Apache-2.0
+github.com/mitchellh/go-homedir,https://github.com/google/go-licenses/blob/HEAD/testdata/modules/vendored03/vendor/github.com/mitchellh/go-homedir/LICENSE,MIT