Add remove_duplicates ignore_comments and merge_comments logic - #132
Add remove_duplicates ignore_comments and merge_comments logic#132jakos-sec wants to merge 2 commits into
remove_duplicates ignore_comments and merge_comments logic#132Conversation
Added `DuplicateResolution` enum to handle advanced deduplication modes. The new mores are `ignore_comments` to keep only the first comment for deduplicated items and `merge_comments` to merge comments from all identical items when deduplicated (identical comment lines are also deduplicated when merging).
wachsler-google
left a comment
wachsler-google
left a comment
There was a problem hiding this comment.
Sorry I don't have much experience with github reviews, so I posted a couple comments separate from this review before I figured out how this works. Anyway, this all looks pretty good, aside from the few things I've commented on.
- Use slices.Clone before deduplication loop when merging comments - Rename remove_duplicates=ignore_comments to keep_first_comment everywhere - Remove duplicate new line - Add tests for remove_duplicates option parsing and resolution
Feedback should be all addressed now. Thank you for the review! |
|
|
||
| if b.metadata.opts.RemoveDuplicates == DuplicateResolutionMergeComments { | ||
| firstLg.comment = slices.Clone(firstLg.comment) | ||
| for _, newComment := range lg.comment { |
There was a problem hiding this comment.
I think you should not be looping over lines and doing a contains check. Doesn't that backfire if there are common lines? Particularly, I think this would strip all subsequent paragraph breaks.
I think this deduping should only ignore comments where all the lines exist in that order (i.e., join the comment strings first).
Can we also add a test case for more complex, multi-line comments, to ensure that removing duplicates correctly handles paragraph breaks and doesn't get confused by similar lines? e.g., like:
// Foo is a solution to all problems.
//
// - It notably does both everything and nothing at the same time.
// - For details on what it does, see
// http://example.com/some-long-url-that-takes-up-the-whole-line
foo
// Foo is the most important value in this list.
//
// A record of past conflicts stemming from this value is available at
// http://example.com/some-long-url-that-takes-up-the-whole-line
foo
Which I think should just result in the following naive appending, since I see those comments to not be duplicates even though they contain individual lines that technically are duplicates.
// Foo is a solution to all problems.
//
// - It notably does both everything and nothing at the same time.
// - For details on what it does, see
// http://example.com/some-long-url-that-takes-up-the-whole-line
// Foo is the most important value in this list.
//
// A record of past conflicts stemming from this value is available at
// http://example.com/some-long-url-that-takes-up-the-whole-line
foo
I didn't run it to check, but I think your code would result in:
// Foo is a solution to all problems.
//
// - It notably does both everything and nothing at the same time.
// - For details on what it does, see
// http://example.com/some-long-url-that-takes-up-the-whole-line
// Foo is the most important value in this list.
// A record of past conflicts stemming from this value is available at
foo
Which IMO is a more confusing result. There's no achieving perfection, but I think merging at a line level rather than the entire comment is just going to confuse.
| Remove duplicates ignoring comments, when the first item has no comment: | ||
| // keep-sorted-test start remove_duplicates=keep_first_comment sticky_comments=yes | ||
| bar | ||
| foo |
There was a problem hiding this comment.
Is this behavior definitely what you want? I'm just not clear on the use case for it. Keeping the first actual comment seems like it would make more sense than letting the no-comment overwrite the duplicate that does have a comment.
WDYT if we change it so that keep_first_comment mode will check if the comment is empty for the first line group and if so, loop over all the line groups until it finds one with a non-empty comment (if any) and copies that comment over to the first line group?
3 participants
Added
DuplicateResolutionenum to handle advanced deduplication modes. The new mores areignore_commentsto keep only the first comment for deduplicated items andmerge_commentsto merge comments from all identical items when deduplicated (identical comment lines are also deduplicated when merging).