Collect Applicable Constraints #1480

JackyChiu · 2017-12-24T17:59:28Z

What does this do / why do we need it?

Makes the collectConstraints method collection only applicable constraints (constraints that are actually imported in the project). Does so by checking project constraints against direct deps.

What should your reviewer look out for in this PR?

Implementation improvements

TODO: Have a maintainer fork test project fixture and then update project root in tests

Do you need help or clarification on anything?

Would like some help in creating a fixture for a test case in TestCollectConstraints where the imports don't use all the constraints. Since it seems like only the maintainers should have their projects in the tests.

Which issue(s) does this PR fix?

fixes #1409

darkowlzz · 2017-12-24T18:14:13Z

@JackyChiu Thanks, I'll review and add some feedback soon. In the meantime, can you look at #302 and see if you can write a function that could help with that issue as well?

And for a test repo, you can create one in your account and I'll fork it from you later.

EDIT: updated the link to the other issue.

JackyChiu · 2017-12-24T18:18:52Z

@darkowlzz Thanks! It's the holidays, so no rush at all!

JackyChiu · 2017-12-25T00:26:26Z

codeclimate is giving a error for the test functions max lines of code and cognitive complexity. Does it make sense to have these rules applied to tests?

darkowlzz · 2017-12-25T18:21:35Z

cmd/dep/status.go

@@ -793,12 +793,23 @@ func collectConstraints(ctx *dep.Ctx, p *dep.Project, sm gps.SourceManager) (con
 			// Get project constraints.
 			pc := manifest.DependencyConstraints()

+			imports, err := projectImports(sm, proj)
+			if err != nil {
+				errCh <- errors.Wrapf(err, "error listing imports used in %s", proj.Ident().ProjectRoot)


If we send this error to the existing error channel, we might end up overflowing the error channel. I would recommend creating another error channel for collecting these errors separately, similar to errListPkgCh and errListVerCh in runStatusAll().

I probably won't need to error after refactoring for the use case above 😄

darkowlzz · 2017-12-25T18:28:33Z

cmd/dep/status.go

@@ -793,12 +793,23 @@ func collectConstraints(ctx *dep.Ctx, p *dep.Project, sm gps.SourceManager) (con
 			// Get project constraints.
 			pc := manifest.DependencyConstraints()

+			imports, err := projectImports(sm, proj)


If I understand this correctly, imports contains all the imports of proj right? Not the root project?
Applicable/working constraints are in respect to the root project, p. That's why getApplicableConstraints() uses rootdata to walk through all the import paths.

Correct me if I'm not seeing it the wrong way 🙂

Opps! My bad, I misunderstood. This makes it simpler though!

I also looked the issue you referred me to, and it does look like a function could be written to satisfy both issues. Which file would the best placement be for it?

darkowlzz · 2017-12-25T18:33:41Z

cmd/dep/status_test.go

+			lock: dep.Lock{
+				P: []gps.LockedProject{
+					gps.NewLockedProject(
+						gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot("github.com/JackyChiu/deptest")},


Let's name the test project something relevant to the test, which would help us recall and refer to it easily. Maybe use words like applicable, constraints, or anything that you think would be accurate. 😊

Sounds good

darkowlzz · 2017-12-25T18:41:26Z

codeclimate is giving a error for the test functions max lines of code and cognitive complexity. Does it make sense to have these rules applied to tests?

@JackyChiu ignore those codeclimate errors for now. Those errors started showing up a few weeks back. Refer https://twitter.com/sdboyer/status/939159860159111169 , not the exact error but I'm sure they have the same origin.

added tests for projectImports collect applicable constraints only test update term used in err msg break up import functions

applicable constrain fixtures

darkowlzz

This looks correct to me. But I would like to wait and ponder on it a bit and maybe have another review.

darkowlzz · 2017-12-26T14:06:02Z

cmd/dep/status_test.go

@@ -305,6 +305,7 @@ func TestCollectConstraints(t *testing.T) {
 	ver1, _ := gps.NewSemverConstraintIC("v1.0.0")
 	ver08, _ := gps.NewSemverConstraintIC("v0.8.0")
 	ver2, _ := gps.NewSemverConstraintIC("v2.0.0")
+	master := gps.NewBranch("master")


This can be used inline. Other's are not inline because they do not return single value.

darkowlzz · 2017-12-26T14:32:35Z

I also looked the issue you referred me to, and it does look like a function could be written to satisfy both issues. Which file would the best placement be for it?

@JackyChiu This would be kinda off-topic. So, manifest.go has ValidateProjectRoots(), where it checks if the project names are actually project root. I think applicable constraints check in manifest can go within that function. And if you want to use getDirectDependencies() there, you would have to take it out of cmd/dep package, which seems fine to me. Might need other's inputs too. So, now this PR and #302 are independent if we go with the approach of using getDirectDependencies().

darkowlzz · 2017-12-26T14:35:20Z

@JackyChiu Also, can you clean up https://github.com/JackyChiu/dep-applicable-constraints/ ? I see a lot of tags in there and that could get confusing later 😅

JackyChiu · 2017-12-26T16:40:35Z

This would be kinda off-topic. So, manifest.go has ValidateProjectRoots(), where it checks if the project names are actually project root. I think applicable constraints check in manifest can go within that function. And if you want to use getDirectDependencies() there, you would have to take it out of cmd/dep package, which seems fine to me. Might need other's inputs too. So, now this PR and #302 are independent if we go with the approach of using getDirectDependencies().

Right that was off topic, and it turned out I didn't need to write a new function lol.

Also, can you clean up https://github.com/JackyChiu/dep-applicable-constraints/ ? I see a lot of tags in there and that could get confusing later 😅

Forgot about all those, cleaned them up

darkowlzz · 2017-12-27T13:48:58Z

I've cloned the test repo https://github.com/darkowlzz/dep-applicable-constraints , you can change the repo names in the tests. And we should be ready with that.

JackyChiu · 2017-12-28T00:47:08Z

@darkowlzz done!

darkowlzz

LGTM

darkowlzz · 2017-12-29T18:10:42Z

Thanks for working on it 👍

JackyChiu requested a review from darkowlzz as a code owner December 24, 2017 17:59

googlebot added the cla: yes label Dec 24, 2017

JackyChiu force-pushed the applicable_constraints branch from 5b317da to 58e2ddf Compare December 24, 2017 18:04

darkowlzz reviewed Dec 25, 2017

View reviewed changes

JackyChiu added 2 commits December 25, 2017 17:16

build import map and then check if constraint match

b209495

added tests for projectImports collect applicable constraints only test update term used in err msg break up import functions

simple check if constrains are directDeps, and updated tests to add

cac9c6b

applicable constrain fixtures

JackyChiu force-pushed the applicable_constraints branch from 6c8a51c to cac9c6b Compare December 25, 2017 22:16

darkowlzz reviewed Dec 26, 2017

View reviewed changes

Inline master branch constraint

725d5c8

JackyChiu changed the title ~~[WIP] Collect Applicable Constraints~~ Collect Applicable Constraints Dec 26, 2017

update test repo fixture name to be under darkowlzz

fb4c3cd

darkowlzz approved these changes Dec 29, 2017

View reviewed changes

darkowlzz merged commit 8d8de2d into golang:master Dec 29, 2017

JackyChiu mentioned this pull request Dec 29, 2017

Collect Applicable Constraints - Test Cleanup #1493

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect Applicable Constraints #1480

Collect Applicable Constraints #1480

JackyChiu commented Dec 24, 2017 •

edited by darkowlzz

Loading

darkowlzz commented Dec 24, 2017 •

edited

Loading

JackyChiu commented Dec 24, 2017

JackyChiu commented Dec 25, 2017

darkowlzz Dec 25, 2017

JackyChiu Dec 25, 2017

darkowlzz Dec 25, 2017

JackyChiu Dec 25, 2017

darkowlzz Dec 25, 2017

JackyChiu Dec 25, 2017

darkowlzz commented Dec 25, 2017

darkowlzz left a comment

darkowlzz Dec 26, 2017

darkowlzz commented Dec 26, 2017

darkowlzz commented Dec 26, 2017

JackyChiu commented Dec 26, 2017

darkowlzz commented Dec 27, 2017

JackyChiu commented Dec 28, 2017

darkowlzz left a comment

darkowlzz commented Dec 29, 2017

Collect Applicable Constraints #1480

Collect Applicable Constraints #1480

Conversation

JackyChiu commented Dec 24, 2017 • edited by darkowlzz Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

darkowlzz commented Dec 24, 2017 • edited Loading

JackyChiu commented Dec 24, 2017

JackyChiu commented Dec 25, 2017

darkowlzz Dec 25, 2017

Choose a reason for hiding this comment

JackyChiu Dec 25, 2017

Choose a reason for hiding this comment

darkowlzz Dec 25, 2017

Choose a reason for hiding this comment

JackyChiu Dec 25, 2017

Choose a reason for hiding this comment

darkowlzz Dec 25, 2017

Choose a reason for hiding this comment

JackyChiu Dec 25, 2017

Choose a reason for hiding this comment

darkowlzz commented Dec 25, 2017

darkowlzz left a comment

Choose a reason for hiding this comment

darkowlzz Dec 26, 2017

Choose a reason for hiding this comment

darkowlzz commented Dec 26, 2017

darkowlzz commented Dec 26, 2017

JackyChiu commented Dec 26, 2017

darkowlzz commented Dec 27, 2017

JackyChiu commented Dec 28, 2017

darkowlzz left a comment

Choose a reason for hiding this comment

darkowlzz commented Dec 29, 2017

JackyChiu commented Dec 24, 2017 •

edited by darkowlzz

Loading

darkowlzz commented Dec 24, 2017 •

edited

Loading