Fix how dep status print revision constraints #1421
Conversation
Before this fix revision constraints would be printed as an
Any constraint ("*") instead of as the revision itself.
cmd/dep/status.go
Outdated
| bs.Constraint = gps.Any() | ||
| for _, c := range cm[bs.ProjectRoot] { | ||
| bs.Constraint = c.Constraint.Intersect(bs.Constraint) | ||
| if pp, has := p.Manifest.Constraints[proj.Ident().ProjectRoot]; has && pp.Constraint != nil { |
There was a problem hiding this comment.
I guess I could collapse this into an else if
if Override {
// ...
} else if Constraint {
// ...
} else {
// Any
}
| @@ -486,6 +486,8 @@ func runStatusAll(ctx *dep.Ctx, out outputter, p *dep.Project, sm gps.SourceMana | |||
| if pp, has := p.Manifest.Ovr[proj.Ident().ProjectRoot]; has && pp.Constraint != nil { | |||
| bs.hasOverride = true | |||
| bs.Constraint = pp.Constraint | |||
| } else if pp, has := p.Manifest.Constraints[proj.Ident().ProjectRoot]; has && pp.Constraint != nil { | |||
There was a problem hiding this comment.
The comment above this block talks about checking in Override. Since with this we check Constraints separately, let's add a comment about that too.
| Constraint: gps.Revision("ddeb6f5d27091ff291b16232e99076a64fb375b8"), | ||
| }, | ||
| wantConstraint: "ddeb6f5", | ||
| }, |
There was a problem hiding this comment.
Thanks for adding this. But we need an integration test to test the actual change in result from this PR, because the runStatusAll() is not run by any of the unit tests. You can refer https://github.com/golang/dep/blob/master/cmd/dep/testdata/harness_tests/README.md to learn how to write and run the integration tests.
There was a problem hiding this comment.
Sounds good. I'll work on the integration test
|
@darkowlzz I've made the requested changes and verified that the new integration test will fail if my code change is removed. It looks like the code climate tool doesn't like the unit test I added earlier, but believe the test is unique enough to keep and that there isn't any valuable refactoring that could be done to prevent the test from triggering the similar code block check. That said, let me know if you want me to remove the unit test since it isn't really testing the change I'm making in this PR. |
|
@otoolec I've been seeing these codeclimate issues a lot recently and nothing has changed on our end. And from this tweet, it looks like something has changed in codeclimate and they are working on a fix. So, it's fine for now. I ran their cli tool locally to analyze the code and it showed uncountable number of results related to duplication. If it's actually an issue, we can fix them separately. Not related to this issue. Also, I overlooked your question about the loop,
Yes, we need that to set BasicStatus Constraint for projects that are not in the manifest but exist in the project. Test looks great. Thank you for fixing this. |
What does this do / why do we need it?
Before this fix revision constraints would be printed as an
Any constraint ("*") instead of as the revision itself.
For example, if my
Gopkg.tomlcontains the following:Then running
dep statuswill print a*for the CONSTRAINT column.But what I would have expected is that it list the constraint in my
Gopkg.tomlfile.What should your reviewer look out for in this PR?
Please make sure that the fix I provided is in the right place and that there wasn't some good reason that
dep statuswasn't listing the constraint the way I was expecting it to.Do you need help or clarification on anything?
Should the following loop be run after my new code as well, or only after setting
gps.Any()?Which issue(s) does this PR fix?
None that I know of.