Skip to content

Add remove_duplicates ignore_comments and merge_comments logic - #132

Open
jakos-sec wants to merge 2 commits into
google:mainfrom
jakos-sec:remove-duplicates-comments
Open

Add remove_duplicates ignore_comments and merge_comments logic#132
jakos-sec wants to merge 2 commits into
google:mainfrom
jakos-sec:remove-duplicates-comments

Conversation

@jakos-sec

Copy link
Copy Markdown

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).

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).
Comment thread keepsorted/block.go Outdated
Comment thread keepsorted/options.go
Comment thread keepsorted/options_parser.go Outdated

@wachsler-google wachsler-google left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@jakos-sec

Copy link
Copy Markdown
Author

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.

Feedback should be all addressed now. Thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants