-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(ensure): concurrent update args validation #1175
fix(ensure): concurrent update args validation #1175
Conversation
cmd/dep/ensure.go
Outdated
// Log all the errors | ||
if len(errArgsValidationCh) > 0 { | ||
for err := range errArgsValidationCh { | ||
ctx.Err.Println(err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we print a line to give some context to these errors?
Something like: Invalid arguments passed to ensure -update:
cdfe942
to
9683e43
Compare
Made the error output similar to manifest validation output with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cmd/dep/ensure.go
Outdated
|
||
func validateUpdateArgs(ctx *dep.Ctx, args []string, p *dep.Project, sm gps.SourceManager, params *gps.SolveParameters) error { | ||
// Channel for receiving all the valid arguments | ||
validArgsCh := make(chan string, len(args)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this argsCh
.
cmd/dep/ensure.go
Outdated
validArgsCh := make(chan string, len(args)) | ||
|
||
// Channel for receiving all the validation errors | ||
errArgsValidationCh := make(chan error, len(args)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this errsCh
.
Apart from the variable names (and they are just an opinion) this LGTM and can be merged. |
Separating the validation code would enable testing the validation code in isolation.
9683e43
to
ebf6207
Compare
What does this do / why do we need it?
Performs
ensure -update
arguments validation concurrently.Related to TODOs:
What should your reviewer look out for in this PR?
Concurrent code implementation.