-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: add debugging logs [CFG-1582] #10
Conversation
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.
Excellent work @teodora-sandu
I've left a nit comment and a few small questions, nothing blocking and this looks good to me ✅ 🚀
debugging := "" | ||
if ok { | ||
for _, e := range customError.errors { | ||
debugging = fmt.Sprintf("%s\n%s", debugging, e.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.
nit: It looks like in the current implementation, we always add an initial newline character.
Could I suggest we instead first aggregate the error messages into a slice with the for
loop, then we use the strings.Join
function with the newline character?
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.
The newline is purposeful - have a look at the screenshots in snyk/cli#2769. I want every error to be one line
errors.New("Error2"), | ||
}, | ||
}) | ||
assert.Equal(t, "\nError1\nError2", actual) |
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.
nit: A few small questions about the logs' content:
Would we perhaps want to have the additional error properties available in these logs somehow? Like the calls stack for example?
Should these logs only include 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.
I don't think they'd be useful and might even end up cluttering the --debug
output too much. If we eventually want to add the stack trace we can, but for now it's more useful to get the actual error message.
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.
Since this is being called from TypeScript using gopherjs
it's a bit complicated to add additional metadata (the easier way to do this is to return a map[string]string
like we do here, otherwise the gopherjs
interface will struggle to parse the nested values)
983fdab
to
963ca73
Compare
963ca73
to
49c75f0
Compare
This PR exposes a new return value from the
ParseModule
function, namelydebugLogs
. This contains extra debugging logs from the parsing, split by file name. The logs can be used in thesnyk
CLI to provide more context to the user about the error.The debug logs are only populated for user errors, which we consider actionable.
CFG-1582