src-d / lookout-gometalint-analyzer

GNU Affero General Public License v3.0
1 stars 5 forks source link

Two dupes found; one of them is not #25

Closed dpordomingo closed 5 years ago

dpordomingo commented 5 years ago

The analyzer returns two suggestions for the same line; one is ok, the other is not: (e.g. at https://github.com/src-d/lookout/pull/442/commits/52e1fa8a904ae92161e0dd977e46ed4428d2a723#r244591876 )

given this piece of code:

    getAnalyzerNotifier := func(ctx context.Context, settings map[string]interface{}) (analyzerNotifier, error) {
        st := grpchelper.ToPBStruct(settings)
        switch ev := e.(type) {
        case *lookout.ReviewEvent:
            if st != nil {
                ev.Configuration = *st
            }
            return analyzerNotifier{
                func(ctx context.Context, a lookout.AnalyzerClient) (*pb.EventResponse, error) {
                    return a.NotifyReviewEvent(ctx, ev)
                }, s.analyzerReviewTimeout}, nil
        case *lookout.PushEvent:
            if st != nil {
                ev.Configuration = *st
            }
            return analyzerNotifier{
                func(ctx context.Context, a lookout.AnalyzerClient) (*pb.EventResponse, error) {
                    return a.NotifyPushEvent(ctx, ev)
                }, s.analyzerPushTimeout}, nil
        default:
            ctxlog.Get(ctx).Debugf("ignoring unsupported event: %s", ev)
            return analyzerNotifier{}, fmt.Errorf("unsupported event: %s", ev)
        }
    }

the analyzer returned

case *lookout.ReviewEvent:

image The first comment points to this block of code:

                    return a.NotifyReviewEvent(ctx, ev)
                }, s.analyzerReviewTimeout}, nil
        case *lookout.PushEvent:
            if st != nil {
                ev.Configuration = *st
            }
            return analyzerNotifier{
                func(ctx context.Context, a lookout.AnalyzerClient) (*pb.EventResponse, error) {

what is not true

And the second comment points to:

        case *lookout.PushEvent:
            if st != nil {
                ev.Configuration = *st
            }
            return analyzerNotifier{
                func(ctx context.Context, a lookout.AnalyzerClient) (*pb.EventResponse, error) {
                    return a.NotifyPushEvent(ctx, ev)
                }, s.analyzerPushTimeout}, nil

what is ok.

smacker commented 5 years ago

Code was re-pushed after the comments were posted. You were looking at the wrong revision. The correct one is here: https://github.com/src-d/lookout/commit/c7aeb3e37d4d1487ecd548356d97793c0c669eef

Both comments point to correct places. You also can see it is correct on PR page:

screenshot 2019-01-10 at 16 25 56