Improve logic for posted comments filter

[smacker]

Currently, we filter out comments with the same text posted on the same line. It works great most of the time. But not in case of sequential commits which "move" changes from the previous commits.
Good example: #442

I propose to improve this logic by also checking:

• remember line number "from" and the line content
• check the content of new line
• if they are the same: skip the comment

Example in screenshots:

First comment:

We can skip this one if apply logic from above:

[dpordomingo]

I like the proposal; @carlosms should we consider it, or do you have any other idea?

[se7entyse7en]

I agree with the fact that the same comment posted twice on different pushes is annoying, but only if it was resolved on the first push. If the issues was not resolved and in the next push that issue was not addressed, then there's the risk that it was simply ignored or forgotten.

I'm also thinking of another scenario. Let's suppose that the first push contains this code:

func FooInc() {
	fn := func(x int) int {  // <--- Lookout-bot: "This line has something I don't like"
		return x + 1
	}
	Bar(fn)
}

func Bar(fn func(int) int) {
	fmt.Println(fn(0))
}

with the second line commented by the bot. Now let's say that another push occurs and now the code contains:

func FooInc() {
	fn := func(x int) int {  // <--- 1
		return x + 1
	}
	Bar(fn)
}

func FooDec() {
	fn := func(x int) int {  // <--- 2
		return x - 1
	}
	Bar(fn)
}

func Bar(fn func(int) int) {
	fmt.Println(fn(0))
}

Given that the two marked lines are identical what should we do in this case? Should we skip the comment for both lines? Should we comment both liens? In my opinion we should skip in the first case and notify in the second case. But still only if in the first case it was resolved as I previously said.

I know that this is an artificial example and it could actually never happen and that the strategy of remembering the line number is already an improvement, and maybe there are also other not too complex improvements that we could apply. I'm just want to say that we should consider a trade-off between false positives and false negatives.

[dpordomingo]

From your first answer:

same comment posted twice on different pushes is annoying, but only if it was resolved or addressed on the first push; if it was not, then there's the risk that it was simply ignored or forgotten.

That's also true. It can be risky to skip comments placed in different lines, without knowing what happened with the first comment (if it was addressed, manually marked as resolved...).

From your example:
considering that the line numbers of the first function are the same in both pushes, source{d} Lookout is already skipping the first comment in the second push because it shares with the one in the first push:

• same file,
• same line,
• same text

(from store/db.go::L236)

[se7entyse7en]

Ok, yes I understood that (but thanks for pointing where it's being done 👍). But I was referring to what should we do in case that in the secondo push both lines are in different position than the first push.