Remove code duplication

So we can enable code duplication check in CI

Added 1 commit:

• c95bc1fb - Use delegate instead of Forwardable

@dzaporozhets Please hold off on merging this one for a bit, @DouweM and I are (perpetually) making reference filter changes.

I'm also a bit worried this is maybe too much abstraction. When I first added these filters I purposefully didn't remove every last bit of duplication, with the goal of making each filter largely self-contained.

What do you think, @DouweM?

Edit: I also hate the class << self syntax, but that's another argument :)

Added 1 commit:

• ce17d769 - Remove duplication in reference filters

@rspeicher I did a rebase so review this commit - ce17d7693b3c81c21bcabbcbb41e389127da93d0.

I am trying to enable code duplication check in CI. For it to pass I need to refactor most violent code duplication. Reference filter is one of it because 3 classes (issue, snippet, mr) shares basically 80% of code.

Added 27 commits:

• ce17d769...0061143c - 22 commits from branch master
• da26fd3f - Dont allow code duplication check to fail
• adec8c77 - Refactor select2 tags
• 79fa6c64 - Remove duplication in reference filters
• 9573e06e - Allow flay to fail for now since there is still a lot of refactoring todo
• bfbfa3b5 - Remove duplication in issue emails

11 errors left and we can enable code duplication check in CI

→ be rake flay
Total score (lower is better) = 1008

1) Similar code found in :defn (mass = 150)
  app/mailers/emails/notes.rb:3
  app/mailers/emails/notes.rb:18
  app/mailers/emails/notes.rb:33

2) IDENTICAL code found in :defn (mass*2 = 124)
  app/controllers/dashboard_controller.rb:13
  app/controllers/groups_controller.rb:62

3) Similar code found in :iter (mass = 106)
  app/services/notification_service.rb:282
  app/services/notification_service.rb:306

4) Similar code found in :defn (mass = 99)
  app/models/project_services/ci/hip_chat_service.rb:47
  app/models/project_services/ci/mail_service.rb:45
  app/models/project_services/ci/slack_service.rb:46

5) Similar code found in :defn (mass = 99)
  app/models/project_services/slack_service/note_message.rb:53
  app/models/project_services/slack_service/note_message.rb:60
  app/models/project_services/slack_service/note_message.rb:67

6) Similar code found in :defn (mass = 92)
  app/helpers/gitlab_markdown_helper.rb:48
  app/helpers/gitlab_markdown_helper.rb:67

7) Similar code found in :if (mass = 74)
  app/services/issues/update_service.rb:18
  app/services/merge_requests/update_service.rb:28

8) Similar code found in :defn (mass = 70)
  lib/gitlab/markdown/user_reference_filter.rb:105
  lib/gitlab/markdown/user_reference_filter.rb:113

9) Similar code found in :module (mass = 68)
  lib/gitlab/markdown/merge_request_reference_filter.rb:3
  lib/gitlab/markdown/snippet_reference_filter.rb:3

10) Similar code found in :block (mass = 66)
  app/controllers/projects/blob_controller.rb:34
  app/controllers/projects/blob_controller.rb:59

11) Similar code found in :defn (mass = 60)
  app/helpers/diff_helper.rb:134
  app/helpers/diff_helper.rb:145

@rspeicher I'm OK with this particular abstraction, the three filters were eerily similar.

Added 3 commits:

• 5f239093 - Remove code duplication in gitlab_markdown_helper.rb
• 437d4c76 - Remove duplication in diff_helper.rb
• 3cebe9e7 - Refactor duplciate code for groups_controller.rb and slack_service/note_message.rb

Added 3 commits:

• 4747412a - Set higher flay value to avoid unnecessary refactoring for now
• 616675b4 - Remove duplication in mailers/emails/notes.rb
• 9b0efdb7 - Remove code duplication in notification_service.rb

Title changed from WIP Remove code duplication to Remove code duplication

Added 3 commits:

• 84b5d035 - Refactor similar code for Issue and MR update service
• 49e32cb5 - Remove small code duplication in user_reference_filter.rb
• fab04fac - Code duplication check should be enabled now

I finished refactoring. That means rake flay should be green now and all new contributions will be checked on code duplication by CI. This should save us a lot of time on code review and prevent low code quality in master.

@rspeicher @DouweM can you please review it? The refactoring itself was not a goal so I tried to avoid unnecessary changes.

@dzaporozhets LGTM!

@DouweM thanks.

@rspeicher waiting for your approve to merge :)

@dzaporozhets LGTM.

Status changed to merged