Improves logging for unknown bblfsh driver in parse cmd #186
Conversation
b841fbb to
11a61d4
Compare
There was a problem hiding this comment.
LGTM but some cosmetic things that could (or should, idk) be addressed.
I see that this PR adds some useful info to the logged error, and it's nice; nevertheless, I do not know what @carlosms required when he opened src-d/sourced-ce#163 because he only said:
the error could be more user-friendly
I'd expect more details about how to make the log more user-friendly, and how this PR helps in that direction.
| ctx, cancel := context.WithTimeout(ctx, time.Second) | ||
| defer cancel() | ||
|
|
||
| res, err := client.Parse(ctx, &api.ParseRequest{ |
There was a problem hiding this comment.
I wonder if it is needed to wrap the original error as returned by client.Parse; if it is not, we could just do:
| res, err := client.Parse(ctx, &api.ParseRequest{ | |
| return client.Parse(ctx, &api.ParseRequest{ |
and get rid of the error checking.
There was a problem hiding this comment.
Actually I don't know the type of error message thrown by the Parse method, so I cannot say whether writing server error gives some more context.
There was a problem hiding this comment.
related to src-d/sourced-ce#186 (comment)
I saw that how we use parseLang is with:
lang, err = parseLang(ctx, c, path, b)
if err != nil {
return fmt.Errorf("cannot parse language: %v", err)
}and in parseLang:
func parseLang(ctx context.Context, client api.EngineClient, path string, b []byte) (string, error) {
// ...
res, err := client.Parse(ctx, &api.ParseRequest{})
if err != nil {
return "", fmt.Errorf("server error: %v", err)
}
return res.Lang, nil
}so imo parseLang is not handling the error, nor dealing with different error types, nor adding any other valuable info but an extra server error string. In that situation I think it's better to return the same error, and let the caller to decide how it's already doing.
There was a problem hiding this comment.
What I meant is that the string server error: <error> may give the user the idea that the type of error is due to some functionality that went wrong during a client-server communication. I don't know if the user might be interested in this kind of information.
I'm thinking about an error such as permission denied. In that case reading cannot parse language: server error: permission denied IMO let the user infer that there's some sort of communication that went wrong. On the other hand simply reading cannot parse language: permission denied might be more cryptic. Again I don't know what type of errors client.Parse may raise, maybe adding the extra server error: string is actually non-necessary and redundant.
There was a problem hiding this comment.
I'm ok with the string; my point is that imo wrapping the error with new strings from every function it crosses, can be wrong.
But it's just an internal feeling, so not a strong opinion in this case ;)
11a61d4 to
232c2a3
Compare
Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
232c2a3 to
8e222c2
Compare
| @@ -79,6 +81,24 @@ The remaining nodes are printed to standard output in JSON format.`, | |||
| return err | |||
| } | |||
|
|
|||
| if lang == "" { | |||
| lang, err = parseLang(ctx, c, path, b) | |||
There was a problem hiding this comment.
I'm not sure we should use parse method of bblfsh file just to get the language of it.
I would better call enry here directly.
Or kept as it was before but check for ErrMissingDriver if bblfsh exports it. (if not, I think we can create an issue to bblfsh).
There was a problem hiding this comment.
Given that currently when the --lang flag is provided the language detection is skipped and performed otherwise, the idea is perform the language detection as a separate step in order to check whether the language is supported. Other than that I don't know which is the best solution for achieving this, and maybe enry (which I don't know) is actually the best option. I used bblfsh just because the logic was already present.
There was a problem hiding this comment.
I'm gonna check that ErrMissingDriver error.
There was a problem hiding this comment.
It seems that by simply checking whether the error thrown is of type ErrMissingDriver this PR could be simplified a lot. I'm just trying to understand this error that I encountered when importing bblfshd.
There was a problem hiding this comment.
No need to import bblfshd. The error should be exported in client-go. If it's not, I think we can request it. The use case for treating this error differently from just "internal error" is common in my opinion.
| if err != nil { | ||
| return fmt.Errorf("server error: %v", err) | ||
| return fmt.Errorf("cannot parse language: %v", err) |
There was a problem hiding this comment.
huh? You parse a file here.
There was a problem hiding this comment.
What I meant is that it cannot parse the language from the file. Do you think that "cannot parse language from given file: %v" or "cannot parse file: %v" is better?
There was a problem hiding this comment.
ah, wait. It's my bad. The context was collapsed on GH and I misunderstood it. All fine. Thanks!
There was a problem hiding this comment.
Since src-d/sourced-ce#186 (comment) is not a strong opinion, and the rest of the comments were already addressed, LGTM
Closes #163.