Better review comments #356
Conversation
| @@ -111,7 +115,7 @@ func (s *Server) HandleEvent(ctx context.Context, e lookout.Event) error { | |||
| } | |||
|
|
|||
| // HandleReview sends request to analyzers concurrently | |||
| func (s *Server) HandleReview(ctx context.Context, e *lookout.ReviewEvent) error { | |||
| func (s *Server) HandleReview(ctx context.Context, e *lookout.ReviewEvent, safePosting bool) error { | |||
There was a problem hiding this comment.
duplicate of server/server.go:166-210 (dupl)
If you have feedback about this comment, please, tell us.
|
Looks good to me. On one hand seems a bit complex for an uncommon problem where the worst that can happen is having duplicated, but valid, comments. On the other hand, since this should only happen very rarely, I guess the added complexity does not affect the common code path and it's ok to handle this corner case in the proper way. I'm not sure about merging comments. If they come from different analyzers, having them as independent comments allows to have 2 conversation threads, or resolve one of them as a bad suggestion but keep discussing the other one. Maybe it would make sense to merge comments that come only from the same analyzer. |
true. The only change for normal code path - writing one more status to DB.
good point! |
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
|
ping @carlosms (rebased on master) |
Fix #280 but differently from the description in the issue.
Working on the original proposal I realized it would require to complicate interfaces a lot.
Poster must return remoteID, add new methods.
Storage interfaces become much more complicated due to the need for transactions across different storages.
Here is different proposal implementation:
There are few more fixes: