replace versionInworkspace() stub #27

jessfraz · 2016-12-03T00:58:22Z

sdboyer · 2016-12-03T14:18:50Z

aaawesome, glad to see this move.

a few things to consider here:

remote/canonical does not have local data (unpushed tags). dvcs only
local data is modified vs remote/canonical (unpushed commits on a branch). dvcs only
dirty working tree. that might mean uncommitted changes, or entirely untracked files

maybe we just ignore these entirely. maybe we put in a general disclaimer note to the user that such things will trip up the init importer.

jessfraz · 2016-12-05T21:19:24Z

added some initial tests

jessfraz · 2016-12-05T23:21:47Z

so this is big because i added tests

sdboyer · 2016-12-06T00:56:53Z

context.go

+
+func newContext() *ctx {
+	// this way we get the default GOPATH that was added in 1.8
+	buildContext := build.Default


Why the additional assignment?

build.Default is calling a function https://github.com/golang/go/blob/master/src/go/build/build.go#L257

OK, but build.Default is the value returned from that function call, not itself a function, so this seems unnecessary?

Or is this a way of guarding against an out-of-order initialization problem, since both of these assignments end up being made as a result of function calls on package var init?

I was more trying to protect us, if we wanted other things from buildContext, from re-calling that function

OK, nbd anyway, it's really just a nit. The thing actually worth discussing is the gopath/splitlist stuff, i think

sdboyer · 2016-12-06T01:04:51Z

init.go

-	case "github.com/termie/go-shutil":
-		return gps.NewBranch("master").Is("4239b77079c7b5d1243b7b4736304ce8ddb6f0f2"), nil
+func (c *ctx) versionInWorkspace(root gps.ProjectRoot) (gps.Version, error) {
+	pr, err := c.absoluteProjectRoot(string(root))


So, I realize you're just doing a a 1:1 change from before, but by moving these methods onto the ctx object, it seems like we need to do away with the loop over all the possible GOPATH workspaces. Instead, we should do it in newContext(), and pick exactly one GOPATH to use then and there, based on the current working directory.

That way these method calls will always and only search the one workspace, which I think is our desired behavior for all the methods on there.

That'd also let us do away with the second return param from determineProjectRoot, as that state would be carried in the ctx object itself.

sdboyer · 2016-12-06T01:08:22Z

init.go

+		return nil, errors.Wrapf(err, "determine project root for %s", root)
+	}
+
+	repo, err := vcs.NewRepo("", string(pr))


Hah...I'm glad an empty first param works. I don't think they'd ever anticipated it, but if it does...awesome.

sdboyer · 2016-12-06T01:10:56Z

init_test.go

+
+func TestIsStdLib(t *testing.T) {
+	tests := map[string]bool{
+		"github.com/sirupsen/logrus": false,


supernit: s/sirupsen/Sirupsen/

(i think he actually switched his own nick to be uncapitalized recently, but we should be consistent in how we refer to it in the tests)

adg · 2016-12-06T00:58:10Z

init.go

+	gopath := os.Getenv("GOPATH")
+	for _, gp := range filepath.SplitList(gopath) {
+		posspath := filepath.Join(gp, "src", path)
+		f, err := os.Stat(posspath)


I'd make an isDir function for this

adg · 2016-12-06T00:59:39Z

init.go

+		return nil, errors.Wrapf(err, "finding current branch/version for root: %s", pr)
+	}
+
+	sha, err := repo.Version()


maybe 'rev' ?

adg · 2016-12-06T01:05:15Z

init_test.go

+
+func TestInit(t *testing.T) {
+	// test must also have external network
+	if _, err := exec.LookPath("git"); err != nil {


maybe we need a needsGit method at some point

adg · 2016-12-06T01:09:48Z

init.go

+//
+// The second returned string indicates which GOPATH value was used.
+// TODO: rename this appropriately
+func (c *ctx) determineProjectRoot(path string) (string, string, error) {


These should be colocated with the ctx type declaration.

adg · 2016-12-06T01:10:54Z

sm.go

-func getSourceManager() (*gps.SourceMgr, error) {
-	gopath := os.Getenv("GOPATH")
-	if gopath == "" {
+func (c *ctx) getSourceManager() (*gps.SourceMgr, error) {


rename 'sourceManager'

adg · 2016-12-06T01:12:09Z

init_test.go

+	tg.setenv("GOPATH", tg.path("."))
+	depCtx := &ctx{GOPATH: tg.path(".")}
+
+	importPaths := map[string]bool{


should also have a case for a file where a dir is expected, or something. if that's possible

adg · 2016-12-06T01:13:16Z

init_test.go

@@ -70,6 +72,39 @@ func TestIsStdLib(t *testing.T) {
 	}
 }

+func TestVersionInWorkspace(t *testing.T) {
+	// test must also have external network
+	if _, err := exec.LookPath("git"); err != nil {


another usage for that helper

Signed-off-by: Jess Frazelle <[email protected]>

Moved all functions that use ctx to context.go Created a test helper for `needsGit` and `needsExternalNetwork` Added a test case for `absoluteProjectPath` to make sure a file will error out Renamed `getSourceManager` to `sourceManager` Signed-off-by: Jess Frazelle <[email protected]>

jessfraz · 2016-12-06T02:01:16Z

Addressed all comments except this one: #27 (comment) which I'm hoping in pairing with Sam tomorrow I can get an idea of what he's thinking :)

sdboyer · 2016-12-06T02:15:49Z

IMO we can merge and fix in pairing tomorrow - no need to bloat this PR more :)

jessfraz · 2016-12-06T02:17:18Z

sweet yeah thats what I was hoping for figured we could just do it together real fast

sdboyer suggested changes Dec 6, 2016

View reviewed changes

adg approved these changes Dec 6, 2016

View reviewed changes

jessfraz added 8 commits December 5, 2016 17:46

replace versionInworkspace() stub

9d0807f

Signed-off-by: Jess Frazelle <[email protected]>

add initial infrastructure for integration testing

b232332

Signed-off-by: Jess Frazelle <[email protected]>

add initial unit tests for init

594ffc5

Signed-off-by: Jess Frazelle <[email protected]>

add initial init test

0db465e

Signed-off-by: Jess Frazelle <[email protected]>

move all functions that check GOPATH to context

2433ec2

Signed-off-by: Jess Frazelle <[email protected]>

add tests for absoluteProjectRoot()

d2341dc

Signed-off-by: Jess Frazelle <[email protected]>

add test for versionInWorkspace()

4413a5d

Signed-off-by: Jess Frazelle <[email protected]>

sdboyer approved these changes Dec 6, 2016

View reviewed changes

jessfraz merged commit 35349ab into golang:master Dec 6, 2016

jessfraz deleted the replace-versionInWorkspace-stub branch December 6, 2016 02:17

replace versionInworkspace() stub #27

replace versionInworkspace() stub #27

Uh oh!

Conversation

jessfraz commented Dec 3, 2016

Uh oh!

sdboyer commented Dec 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jessfraz commented Dec 5, 2016

Uh oh!

jessfraz commented Dec 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessfraz Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessfraz commented Dec 6, 2016

Uh oh!

sdboyer commented Dec 6, 2016

Uh oh!

jessfraz commented Dec 6, 2016

Uh oh!

Uh oh!

sdboyer commented Dec 3, 2016 •

edited

Loading

jessfraz Dec 6, 2016 •

edited

Loading