-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmd/validate: print the validation errors #163
Conversation
de5f967
to
644ab35
Compare
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.
One minor comment. Not a blocker though.
cmd/cdi/cmd/validate.go
Outdated
for path, specErrors := range cdiErrors { | ||
fmt.Printf("Spec file %s:\n", path) | ||
for idx, err := range specErrors { | ||
fmt.Printf(" %d: %v\n", idx, strings.TrimRight(err.Error(), "\n")) |
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.
What about just:
fmt.Printf(" %d: %v\n", idx, strings.TrimRight(err.Error(), "\n")) | |
fmt.Printf(" %d: %v\n", idx, strings.Trim(err.Error())) |
We should be ok to just ignore all leading and trailing whitespace in error messages.
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.
Makes sense. Changed it to strings.TrimSpace()
374b1d9
to
e2171b1
Compare
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.
minor nit but not a blocker.
cmd/cdi/cmd/validate.go
Outdated
for path, specErrors := range cdiErrors { | ||
fmt.Printf("Spec file %s:\n", path) | ||
for idx, err := range specErrors { | ||
fmt.Printf(" %d: %v\n", idx, strings.TrimSpace(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.
Thanks for the update. As a matter of interest, do we want to use tabs instead of spaces here so that the columns are aligned? This really isn't a blocker though, and I'm happy to approve as is.
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.
Instead of using tabs I now changed this to %3d
so it will reserve 3 chars for the idx. Will get unaligned if there are more than 999 errors
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.
Actually, that would be 999 errors per spec. I changed it to %2d
so it will get unaligned after 99 errors per spec
e2171b1
to
2c3c447
Compare
Print out any errors encountered during validation, as suggested by the command help. Signed-off-by: Markus Lehtonen <[email protected]>
2c3c447
to
1078daf
Compare
Print out any errors encountered during validation, as suggested by the command help.