Add constructors for feedback logs

The feedback system was originally designed for reporting feedback as GOPATH
was scanned, which didn’t work well for logging from the importers. This moves
more of the logic around what should be logged back onto the caller and simplifies
the ConstraintFeedback.LogFeedback function to print what it is given.

cmd/dep/glide_importer.go

@@ -208,6 +208,9 @@ func (g *glideImporter) buildProjectConstraint(pkg glidePackage) (pc gps.Project
 	pc.Ident = gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot(pkg.Name), Source: pkg.Repository}
 	pc.Constraint, err = deduceConstraint(pkg.Reference, pc.Ident, g.sm)
 
+	f := fb.NewConstraintFeedback(pc, fb.DepTypeImported)
+	f.LogFeedback(g.logger)
+
 	return
 }
 
@@ -226,6 +229,10 @@ func (g *glideImporter) buildLockedProject(pkg glideLockedPackage) gps.LockedPro
 		version = revision
 	}
 
-	feedback(version, pi.ProjectRoot, fb.DepTypeImported, g.logger)
-	return gps.NewLockedProject(pi, version, nil)
+	lp := gps.NewLockedProject(pi, version, nil)
+
+	f := fb.NewLockedProjectFeedback(lp, fb.DepTypeImported)
+	f.LogFeedback(g.logger)
+
+	return lp
 }

cmd/dep/godep_importer.go

@@ -159,6 +159,10 @@ func (g *godepImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, e
 func (g *godepImporter) buildProjectConstraint(pkg godepPackage) (pc gps.ProjectConstraint, err error) {
 	pc.Ident = gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot(pkg.ImportPath)}
 	pc.Constraint, err = deduceConstraint(pkg.Comment, pc.Ident, g.sm)
+
+	f := fb.NewConstraintFeedback(pc, fb.DepTypeImported)
+	f.LogFeedback(g.logger)
+
 	return
 }
 
@@ -175,8 +179,11 @@ func (g *godepImporter) buildLockedProject(pkg godepPackage) gps.LockedProject {
 		version = gps.Revision(pkg.Rev)
 	}
 
-	feedback(version, pi.ProjectRoot, fb.DepTypeImported, g.logger)
-	return gps.NewLockedProject(pi, version, nil)
+	lp := gps.NewLockedProject(pi, version, nil)
+	f := fb.NewLockedProjectFeedback(lp, fb.DepTypeImported)
+	f.LogFeedback(g.logger)
+
+	return lp
 }
 
 // projectExistsInLock checks if the given import path already existing in

cmd/dep/gopath_scanner.go

@@ -91,7 +91,12 @@ func (g *gopathScanner) overlay(rootM *dep.Manifest, rootL *dep.Lock) {
 		}
 		rootM.Constraints[pkg] = prj
 		v := g.pd.ondisk[pkg]
-		feedback(v, pkg, fb.DepTypeDirect, g.ctx.Err)
+
+		pi := gps.ProjectIdentifier{ProjectRoot: pkg, Source: prj.Source}
+		f := fb.NewConstraintFeedback(gps.ProjectConstraint{Ident: pi, Constraint: v}, fb.DepTypeDirect)
+		f.LogFeedback(g.ctx.Err)
+		f = fb.NewLockedProjectFeedback(gps.NewLockedProject(pi, v, nil), fb.DepTypeDirect)
+		f.LogFeedback(g.ctx.Err)
 	}
 
 	// Keep track of which projects have been locked
@@ -109,8 +114,8 @@ func (g *gopathScanner) overlay(rootM *dep.Manifest, rootL *dep.Lock) {
 		lockedProjects[pkg] = true
 
 		if _, isDirect := g.directDeps[string(pkg)]; !isDirect {
-			v := g.pd.ondisk[pkg]
-			feedback(v, pkg, fb.DepTypeTransitive, g.ctx.Err)
+			f := fb.NewLockedProjectFeedback(lp, fb.DepTypeTransitive)
+			f.LogFeedback(g.ctx.Err)
 		}
 	}
 

cmd/dep/root_analyzer.go

@@ -5,14 +5,12 @@
 package main
 
 import (
-	"encoding/hex"
+	"io/ioutil"
+	"log"
 
 	"github.com/golang/dep"
-	fb "github.com/golang/dep/internal/feedback"
 	"github.com/golang/dep/internal/gps"
 	"github.com/pkg/errors"
-	"io/ioutil"
-	"log"
 )
 
 // importer handles importing configuration from other dependency managers into
@@ -149,50 +147,6 @@ func (a *rootAnalyzer) Info() (string, int) {
 	return name, version
 }
 
-// feedback logs project constraint as feedback to the user.
-func feedback(v gps.Version, pr gps.ProjectRoot, depType string, logger *log.Logger) {
-	rev, version, branch := gps.VersionComponentStrings(v)
-
-	// Check if it's a valid SHA1 digest and trim to 7 characters.
-	if len(rev) == 40 {
-		if _, err := hex.DecodeString(rev); err == nil {
-			// Valid SHA1 digest
-			rev = rev[0:7]
-		}
-	}
-
-	// Get LockedVersion
-	var ver string
-	if version != "" {
-		ver = version
-	} else if branch != "" {
-		ver = branch
-	}
-
-	cf := &fb.ConstraintFeedback{
-		LockedVersion:  ver,
-		Revision:       rev,
-		ProjectPath:    string(pr),
-		DependencyType: depType,
-	}
-
-	// Get non-revision constraint if available
-	if c := getProjectPropertiesFromVersion(v).Constraint; c != nil {
-		cf.Version = c.String()
-	}
-
-	// Attach ConstraintType for direct/imported deps based on locked version
-	if cf.DependencyType == fb.DepTypeDirect || cf.DependencyType == fb.DepTypeImported {
-		if cf.LockedVersion != "" {
-			cf.ConstraintType = fb.ConsTypeConstraint
-		} else {
-			cf.ConstraintType = fb.ConsTypeHint
-		}
-	}
-
-	cf.LogFeedback(logger)
-}
-
 func lookupVersionForRevision(rev gps.Revision, pi gps.ProjectIdentifier, sm gps.SourceManager) (gps.Version, error) {
 	// Find the version that goes with this revision, if any
 	versions, err := sm.ListVersions(pi)

cmd/dep/testdata/glide/expected_import_output.txt

@@ -1,7 +1,7 @@
 Detected glide configuration files...
 Converting from glide.yaml and glide.lock...
-  Using ^0.8.1 as initial constraint for imported dep github.com/sdboyer/deptest
-  Trying v0.8.1 (3f4c3be) as initial lock for imported dep github.com/sdboyer/deptest
+  Using master as initial constraint for imported dep github.com/sdboyer/deptest
   Using ^2.0.0 as initial constraint for imported dep github.com/sdboyer/deptestdos
+  Using master as initial constraint for imported dep github.com/golang/lint
+  Trying v0.8.1 (3f4c3be) as initial lock for imported dep github.com/sdboyer/deptest
   Trying v2.0.0 (5c60720) as initial lock for imported dep github.com/sdboyer/deptestdos
-  Using cb00e56 as initial hint for imported dep github.com/golang/lint

internal/feedback/feedback.go

@@ -5,8 +5,11 @@
 package feedback
 
 import (
+	"encoding/hex"
 	"fmt"
 	"log"
+
+	"github.com/golang/dep/internal/gps"
 )
 
 // Constraint types
@@ -25,23 +28,52 @@ const DepTypeImported = "imported dep"
 
 // ConstraintFeedback holds project constraint feedback data
 type ConstraintFeedback struct {
-	Version, LockedVersion, Revision, ConstraintType, DependencyType, ProjectPath string
+	Constraint, LockedVersion, Revision, ConstraintType, DependencyType, ProjectPath string
 }
 
-// LogFeedback logs the feedback
+// NewConstraintFeedback builds a feedback entry for a constraint in the manifest.
+func NewConstraintFeedback(pc gps.ProjectConstraint, depType string) *ConstraintFeedback {
+	cf := &ConstraintFeedback{
+		Constraint:     pc.Constraint.String(),
+		ProjectPath:    string(pc.Ident.ProjectRoot),
+		DependencyType: depType,
+	}
+
+	if _, ok := pc.Constraint.(gps.Revision); ok {
+		cf.ConstraintType = ConsTypeHint
+	} else {
+		cf.ConstraintType = ConsTypeConstraint
+	}
+
+	return cf
+}
+
+// NewLockedProjectFeedback builds a feedback entry for a project in the lock.
+func NewLockedProjectFeedback(lp gps.LockedProject, depType string) *ConstraintFeedback {
+	cf := &ConstraintFeedback{
+		ProjectPath:    string(lp.Ident().ProjectRoot),
+		DependencyType: depType,
+	}
+
+	switch vt := lp.Version().(type) {
+	case gps.PairedVersion:
+		cf.LockedVersion = vt.String()
+		cf.Revision = vt.Underlying().String()
+	case gps.UnpairedVersion: // Logically this should never occur, but handle for completeness sake
+		cf.LockedVersion = vt.String()
+	case gps.Revision:
+		cf.Revision = vt.String()
+	}
+
+	return cf
+}
+
+// LogFeedback logs feedback on changes made to the manifest or lock.
 func (cf ConstraintFeedback) LogFeedback(logger *log.Logger) {
-	// "Using" feedback for direct dep
-	if cf.DependencyType == DepTypeDirect || cf.DependencyType == DepTypeImported {
-		ver := cf.Version
-		// revision as version for hint
-		if cf.ConstraintType == ConsTypeHint {
-			ver = cf.Revision
-		}
-		logger.Printf("  %v", GetUsingFeedback(ver, cf.ConstraintType, cf.DependencyType, cf.ProjectPath))
+	if cf.Constraint != "" {
+		logger.Printf("  %v", GetUsingFeedback(cf.Constraint, cf.ConstraintType, cf.DependencyType, cf.ProjectPath))
 	}
-	// No "Locking" feedback for hints. "Locking" feedback only for constraint
-	// and transitive dep
-	if cf.ConstraintType != ConsTypeHint {
+	if cf.LockedVersion != "" && cf.Revision != "" {
 		logger.Printf("  %v", GetLockingFeedback(cf.LockedVersion, cf.Revision, cf.DependencyType, cf.ProjectPath))
 	}
 }
@@ -62,6 +94,14 @@ func GetUsingFeedback(version, consType, depType, projectPath string) string {
 // Locking in v1.1.4 (bc29b4f) for direct dep github.com/foo/bar
 // Locking in master (436f39d) for transitive dep github.com/baz/qux
 func GetLockingFeedback(version, revision, depType, projectPath string) string {
+	// Check if it's a valid SHA1 digest and trim to 7 characters.
+	if len(revision) == 40 {
+		if _, err := hex.DecodeString(revision); err == nil {
+			// Valid SHA1 digest
+			revision = revision[0:7]
+		}
+	}
+
 	if depType == DepTypeImported {
 		return fmt.Sprintf("Trying %s (%s) as initial lock for %s %s", version, revision, depType, projectPath)
 	}

internal/feedback/feedback_test.go

@@ -5,47 +5,82 @@
 package feedback
 
 import (
+	"bytes"
+	log2 "log"
+	"strings"
 	"testing"
+
+	"github.com/golang/dep/internal/gps"
 )
 
-func TestGetConstraintString(t *testing.T) {
+func TestFeedback_Constraint(t *testing.T) {
+	ver, _ := gps.NewSemverConstraint("^1.0.0")
+	rev := gps.Revision("1b8edb3")
+	pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/foo/bar")}
+
 	cases := []struct {
-		feedback string
+		feedback *ConstraintFeedback
 		want     string
 	}{
 		{
-			feedback: GetUsingFeedback("^1.0.0", ConsTypeConstraint, DepTypeDirect, "github.com/foo/bar"),
+			feedback: NewConstraintFeedback(gps.ProjectConstraint{Constraint: ver, Ident: pi}, DepTypeDirect),
 			want:     "Using ^1.0.0 as constraint for direct dep github.com/foo/bar",
 		},
 		{
-			feedback: GetUsingFeedback("^1.0.0", ConsTypeConstraint, DepTypeImported, "github.com/foo/bar"),
+			feedback: NewConstraintFeedback(gps.ProjectConstraint{Constraint: ver, Ident: pi}, DepTypeImported),
 			want:     "Using ^1.0.0 as initial constraint for imported dep github.com/foo/bar",
 		},
 		{
-			feedback: GetUsingFeedback("1b8edb3", ConsTypeHint, DepTypeDirect, "github.com/bar/baz"),
-			want:     "Using 1b8edb3 as hint for direct dep github.com/bar/baz",
+			feedback: NewConstraintFeedback(gps.ProjectConstraint{Constraint: rev, Ident: pi}, DepTypeDirect),
+			want:     "Using 1b8edb3 as hint for direct dep github.com/foo/bar",
 		},
 		{
-			feedback: GetUsingFeedback("1b8edb3", ConsTypeHint, DepTypeImported, "github.com/bar/baz"),
-			want:     "Using 1b8edb3 as initial hint for imported dep github.com/bar/baz",
+			feedback: NewConstraintFeedback(gps.ProjectConstraint{Constraint: rev, Ident: pi}, DepTypeImported),
+			want:     "Using 1b8edb3 as initial hint for imported dep github.com/foo/bar",
 		},
+	}
+
+	for _, c := range cases {
+		buf := &bytes.Buffer{}
+		log := log2.New(buf, "", 0)
+		c.feedback.LogFeedback(log)
+		got := strings.TrimSpace(buf.String())
+		if c.want != got {
+			t.Errorf("Feedbacks are not expected: \n\t(GOT) '%s'\n\t(WNT) '%s'", got, c.want)
+		}
+	}
+}
+
+func TestFeedback_LockedProject(t *testing.T) {
+	v := gps.NewVersion("v1.1.4").Is("bc29b4f")
+	b := gps.NewBranch("master").Is("436f39d")
+	pi := gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/foo/bar")}
+
+	cases := []struct {
+		feedback *ConstraintFeedback
+		want     string
+	}{
 		{
-			feedback: GetLockingFeedback("v1.1.4", "bc29b4f", DepTypeDirect, "github.com/foo/bar"),
+			feedback: NewLockedProjectFeedback(gps.NewLockedProject(pi, v, nil), DepTypeDirect),
 			want:     "Locking in v1.1.4 (bc29b4f) for direct dep github.com/foo/bar",
 		},
 		{
-			feedback: GetLockingFeedback("v1.1.4", "bc29b4f", DepTypeImported, "github.com/foo/bar"),
+			feedback: NewLockedProjectFeedback(gps.NewLockedProject(pi, v, nil), DepTypeImported),
 			want:     "Trying v1.1.4 (bc29b4f) as initial lock for imported dep github.com/foo/bar",
 		},
 		{
-			feedback: GetLockingFeedback("master", "436f39d", DepTypeTransitive, "github.com/baz/qux"),
-			want:     "Locking in master (436f39d) for transitive dep github.com/baz/qux",
+			feedback: NewLockedProjectFeedback(gps.NewLockedProject(pi, b, nil), DepTypeTransitive),
+			want:     "Locking in master (436f39d) for transitive dep github.com/foo/bar",
 		},
 	}
 
 	for _, c := range cases {
-		if c.want != c.feedback {
-			t.Errorf("Feedbacks are not expected: \n\t(GOT) %v\n\t(WNT) %v", c.feedback, c.want)
+		buf := &bytes.Buffer{}
+		log := log2.New(buf, "", 0)
+		c.feedback.LogFeedback(log)
+		got := strings.TrimSpace(buf.String())
+		if c.want != got {
+			t.Errorf("Feedbacks are not expected: \n\t(GOT) '%s'\n\t(WNT) '%s'", got, c.want)
 		}
 	}
 }