-
Notifications
You must be signed in to change notification settings - Fork 36
feature: make lookout-sdk return error code if analysis failed #411
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
Conversation
I have rewritten concurrentRequest function using channels to be able to exit as soon as the first analyzer failed. commentList is also replaced with channel because it's weird to mix sync package and channels in one function. fix: #409 Signed-off-by: Maxim Sukharev <max@smacker.ru>
@@ -51,7 +54,7 @@ func NewServer( | |||
reviewTimeout time.Duration, | |||
pushTimeout time.Duration, | |||
) *Server { | |||
return &Server{p, fileGetter, analyzers, eventOp, commentOp, reviewTimeout, pushTimeout} | |||
return &Server{false, p, fileGetter, analyzers, eventOp, commentOp, reviewTimeout, pushTimeout} |
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'm late in the discussion... but... I'd prefer not to use positional arguments.
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.
sorry David but it's unrelated to this PR. It uses the same style that was used before. We can fix it in another PR later if you want.
Signed-off-by: Maxim Sukharev <max@smacker.ru>
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.
👍
Signed-off-by: Maxim Sukharev <max@smacker.ru>
I updated the failing test also. |
I have rewritten concurrentRequest function using channels to be able to
exit as soon as the first analyzer failed.
commentList is also replaced with channel because it's weird to mix
sync package and channels in one function.
fix: #409
Signed-off-by: Maxim Sukharev max@smacker.ru