cmd/dep: make checkErrors more lenient #997
Conversation
|
the basic algoirhtm seems right, but i think it might be better just to keep it simple and encap all the logic in if necessary, we can add another param to |
|
Any suggestions? Should I return two values |
|
I like returning |
|
Or maybe |
cmd/dep/ensure.go
Outdated
| } | ||
|
|
||
| func (e *pkgtreeErrs) Error() string { | ||
| var errors []string |
There was a problem hiding this comment.
nit: could init slice to exact capacity here: errors := make([]string, 0, len(e.errs)) (or only make with a length, and use the index instead of append in the loop)
|
let's go with |
ibrasho
force-pushed
the
update-checkErrors-to-be-more-lenient
branch
2 times, most recently
from
754ab11 to
245f2ab
Compare
|
Updated. |
cmd/dep/ensure.go
Outdated
| return err | ||
| } else if ctx.Verbose { |
There was a problem hiding this comment.
Shouldn't this block only execute if err != nil?
There was a problem hiding this comment.
My bad, there should have been another nested if.
cmd/dep/ensure.go
Outdated
| ctx.Out.Println(errs) | ||
| } else { | ||
| ctx.Out.Println(err) | ||
| } |
There was a problem hiding this comment.
Do these two behave differently?
Zooming out a bit and considering both: is this equivalent (or should it be)?
if fatal, err := checkErrors(params.RootPackageTree.Packages); err != nil {
if fatal {
return err
} else if ctx.Verbose {
ctx.Out.Println(err)
}
}There was a problem hiding this comment.
This was my original intention. 😁
ibrasho
force-pushed
the
update-checkErrors-to-be-more-lenient
branch
3 times, most recently
from
95dbb1d to
b06f843
Compare
|
Updated again. 😁 |
cmd/dep/ensure_test.go
Outdated
| t.Fail() | ||
| fatal, err := checkErrors(tc.pkgOrErrMap) | ||
| if err == nil && fatal { | ||
| t.Fatalf("unexpected fatal flag %T value while err in nil", fatal) |
There was a problem hiding this comment.
s/in nil/is nil
Also, fatal is always true in this block, so the dynamic formatting is not necessary.
ibrasho
force-pushed
the
update-checkErrors-to-be-more-lenient
branch
from
b06f843 to
e0f45d4
Compare
|
And again. 😛 |
cmd/dep/ensure.go
Outdated
| return true, pkgtreeErrors | ||
| } | ||
|
|
||
| // If m contained some errors, return a warning with these errors. |
There was a problem hiding this comment.
nit: these those errors
checkErrors doesn't allow dep ensure to be used in a project that contains any Go files containing build errors. This commit update checkErrors to return a warning instead of an error as long as the project contains some compilable Go files. Fixes golang#995 Signed-off-by: Ibrahim AshShohail <[email protected]>
ibrasho
force-pushed
the
update-checkErrors-to-be-more-lenient
branch
from
e0f45d4 to
b5c378e
Compare
What does this do / why do we need it?
checkErrorscurrently doesn't allowdep ensureto be used in a project that contains any Go files with build errors. This PR updatecheckErrorsto return a warning instead of an error as long as the project contains some compilable Go files.What should your reviewer look out for in this PR?
Does this make sense?
Do you need help or clarification on anything?
Why did we add that check in the first case?
Which issue(s) does this PR fix?
Fixes #995