From e4f59f2ecb0dd9958ab48c50dcd42ee0c6517ea8 Mon Sep 17 00:00:00 2001
From: Kassio Borges <kborges@gitlab.com>
Date: Wed, 29 Sep 2021 18:42:31 +0100
Subject: [PATCH] GithubImporter: Format diff note suggestions to the gitlab
format
Github "suggestion" feature has a different markdown format, it uses:
```suggestion
SUGGESTION
```
While Gitlab has a _range_ in the suggestion markdown, like:
```suggestion-0+0
SUGGESTION
```
To convert the Github format to Gitlab, in the `DiffNote`
_representation_ the range will be added. The range is calculated by the
difference of the `start_line` and `line` when the start_line is
present, which indicates a multi-line suggestion.
Related to: https://gitlab.com/gitlab-org/gitlab/-/issues/340624
Changelog: changed
MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71411
---
.../github_import/representation/diff_note.rb | 12 +-
.../diff_notes/suggestion_formatter.rb | 66 +++++++
.../importer/diff_notes_importer_spec.rb | 12 +-
.../representation/diff_note_spec.rb | 117 ++++++++++---
.../diff_notes/suggestion_formatter_spec.rb | 164 ++++++++++++++++++
5 files changed, 347 insertions(+), 24 deletions(-)
create mode 100644 lib/gitlab/github_import/representation/diff_notes/suggestion_formatter.rb
create mode 100644 spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb
diff --git a/lib/gitlab/github_import/representation/diff_note.rb b/lib/gitlab/github_import/representation/diff_note.rb
index 1d6dfbead5f1c059..a3dcd2e380c6cead 100644
--- a/lib/gitlab/github_import/representation/diff_note.rb
+++ b/lib/gitlab/github_import/representation/diff_note.rb
@@ -40,7 +40,9 @@ def self.from_api_response(note)
note: note.body,
created_at: note.created_at,
updated_at: note.updated_at,
- note_id: note.id
+ note_id: note.id,
+ end_line: note.line,
+ start_line: note.start_line
}
new(hash)
@@ -83,6 +85,14 @@ def diff_hash
}
end
+ def note
+ @note ||= DiffNotes::SuggestionFormatter.formatted_note_for(
+ note: attributes[:note],
+ start_line: attributes[:start_line],
+ end_line: attributes[:end_line]
+ )
+ end
+
def github_identifiers
{
note_id: note_id,
diff --git a/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter.rb b/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter.rb
new file mode 100644
index 0000000000000000..4e5855ee4cdbdf36
--- /dev/null
+++ b/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter.rb
@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+# This class replaces Github markdown suggestion tag with
+# a Gitlab suggestion tag. The difference between
+# Github's and Gitlab's suggestion tags is that Gitlab
+# includes the range of the suggestion in the tag, while Github
+# uses other note attributes to position the suggestion.
+module Gitlab
+ module GithubImport
+ module Representation
+ module DiffNotes
+ class SuggestionFormatter
+ # A github suggestion:
+ # - the ```suggestion tag must be the first text of the line
+ # - it might have up to 3 spaces before the ```suggestion tag
+ # - extra text on the ```suggestion tag line will be ignored
+ GITHUB_SUGGESTION = /^\ {,3}(?<suggestion>```suggestion\b).*(?<eol>\R)/.freeze
+
+ def self.formatted_note_for(...)
+ new(...).formatted_note
+ end
+
+ def initialize(note:, start_line: nil, end_line: nil)
+ @note = note
+ @start_line = start_line
+ @end_line = end_line
+ end
+
+ def formatted_note
+ if contains_suggestion?
+ note.gsub(
+ GITHUB_SUGGESTION,
+ "\\k<suggestion>:#{suggestion_range}\\k<eol>"
+ )
+ else
+ note
+ end
+ end
+
+ private
+
+ attr_reader :note, :start_line, :end_line
+
+ def contains_suggestion?
+ note.to_s.match?(GITHUB_SUGGESTION)
+ end
+
+ # Github always saves the comment on the _last_ line of the range.
+ # Therefore, the diff hunk will always be related to lines before
+ # the comment itself.
+ def suggestion_range
+ "-#{line_count}+0"
+ end
+
+ def line_count
+ if start_line.present?
+ end_line - start_line
+ else
+ 0
+ end
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb b/spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb
index 46b9959ff64f88cc..be4fc3cbf16ff4d7 100644
--- a/spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb
@@ -15,10 +15,18 @@
original_commit_id: 'original123abc',
diff_hunk: "@@ -1 +1 @@\n-Hello\n+Hello world",
user: double(:user, id: 4, login: 'alice'),
- body: 'Hello world',
created_at: Time.zone.now,
updated_at: Time.zone.now,
- id: 1
+ line: 23,
+ start_line: nil,
+ id: 1,
+ body: <<~BODY
+ Hello World
+
+ ```suggestion
+ sug1
+ ```
+ BODY
)
end
diff --git a/spec/lib/gitlab/github_import/representation/diff_note_spec.rb b/spec/lib/gitlab/github_import/representation/diff_note_spec.rb
index 91479d23f43bb01d..81722c0eba740cae 100644
--- a/spec/lib/gitlab/github_import/representation/diff_note_spec.rb
+++ b/spec/lib/gitlab/github_import/representation/diff_note_spec.rb
@@ -73,6 +73,8 @@
body: 'Hello world',
created_at: created_at,
updated_at: updated_at,
+ line: 23,
+ start_line: nil,
id: 1
)
end
@@ -90,47 +92,70 @@
expect(note.author).to be_nil
end
- end
- describe '.from_json_hash' do
- it_behaves_like 'a DiffNote' do
- let(:hash) do
- {
- 'noteable_type' => 'MergeRequest',
- 'noteable_id' => 42,
- 'file_path' => 'README.md',
- 'commit_id' => '123abc',
- 'original_commit_id' => 'original123abc',
- 'diff_hunk' => hunk,
- 'author' => { 'id' => 4, 'login' => 'alice' },
- 'note' => 'Hello world',
- 'created_at' => created_at.to_s,
- 'updated_at' => updated_at.to_s,
- 'note_id' => 1
- }
- end
+ it 'formats a suggestion in the note body' do
+ allow(response)
+ .to receive(:body)
+ .and_return <<~BODY
+ ```suggestion
+ Hello World
+ ```
+ BODY
- let(:note) { described_class.from_json_hash(hash) }
+ note = described_class.from_api_response(response)
+
+ expect(note.note).to eq <<~BODY
+ ```suggestion:-0+0
+ Hello World
+ ```
+ BODY
end
+ end
- it 'does not convert the author if it was not specified' do
- hash = {
+ describe '.from_json_hash' do
+ let(:hash) do
+ {
'noteable_type' => 'MergeRequest',
'noteable_id' => 42,
'file_path' => 'README.md',
'commit_id' => '123abc',
'original_commit_id' => 'original123abc',
'diff_hunk' => hunk,
+ 'author' => { 'id' => 4, 'login' => 'alice' },
'note' => 'Hello world',
'created_at' => created_at.to_s,
'updated_at' => updated_at.to_s,
'note_id' => 1
}
+ end
+
+ it_behaves_like 'a DiffNote' do
+ let(:note) { described_class.from_json_hash(hash) }
+ end
+
+ it 'does not convert the author if it was not specified' do
+ hash.delete('author')
note = described_class.from_json_hash(hash)
expect(note.author).to be_nil
end
+
+ it 'formats a suggestion in the note body' do
+ hash['note'] = <<~BODY
+ ```suggestion
+ Hello World
+ ```
+ BODY
+
+ note = described_class.from_json_hash(hash)
+
+ expect(note.note).to eq <<~BODY
+ ```suggestion:-0+0
+ Hello World
+ ```
+ BODY
+ end
end
describe '#line_code' do
@@ -181,4 +206,54 @@
expect(note.github_identifiers).to eq(github_identifiers)
end
end
+
+ describe '#note' do
+ it 'returns the given note' do
+ hash = {
+ 'note': 'simple text'
+ }
+
+ note = described_class.new(hash)
+
+ expect(note.note).to eq 'simple text'
+ end
+
+ it 'returns the suggestion formatted in the note' do
+ hash = {
+ 'note': <<~BODY
+ ```suggestion
+ Hello World
+ ```
+ BODY
+ }
+
+ note = described_class.new(hash)
+
+ expect(note.note).to eq <<~BODY
+ ```suggestion:-0+0
+ Hello World
+ ```
+ BODY
+ end
+
+ it 'returns the multi-line suggestion formatted in the note' do
+ hash = {
+ 'start_line': 20,
+ 'end_line': 23,
+ 'note': <<~BODY
+ ```suggestion
+ Hello World
+ ```
+ BODY
+ }
+
+ note = described_class.new(hash)
+
+ expect(note.note).to eq <<~BODY
+ ```suggestion:-3+0
+ Hello World
+ ```
+ BODY
+ end
+ end
end
diff --git a/spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb b/spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb
new file mode 100644
index 0000000000000000..2ffd5f50d3ba901c
--- /dev/null
+++ b/spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb
@@ -0,0 +1,164 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormatter do
+ it 'does nothing when there is any text before the suggestion tag' do
+ note = <<~BODY
+ looks like```suggestion but it isn't
+ ```
+ BODY
+
+ expect(described_class.formatted_note_for(note: note)).to eq(note)
+ end
+
+ it 'handles nil value for note' do
+ note = nil
+
+ expect(described_class.formatted_note_for(note: note)).to eq(note)
+ end
+
+ it 'does not allow over 3 leading spaces for valid suggestion' do
+ note = <<~BODY
+ Single-line suggestion
+ ```suggestion
+ sug1
+ ```
+ BODY
+
+ expect(described_class.formatted_note_for(note: note)).to eq(note)
+ end
+
+ it 'allows up to 3 leading spaces' do
+ note = <<~BODY
+ Single-line suggestion
+ ```suggestion
+ sug1
+ ```
+ BODY
+
+ expected = <<~BODY
+ Single-line suggestion
+ ```suggestion:-0+0
+ sug1
+ ```
+ BODY
+
+ expect(described_class.formatted_note_for(note: note)).to eq(expected)
+ end
+
+ it 'does nothing when there is any text without space after the suggestion tag' do
+ note = <<~BODY
+ ```suggestionbut it isn't
+ ```
+ BODY
+
+ expect(described_class.formatted_note_for(note: note)).to eq(note)
+ end
+
+ it 'formats single-line suggestions' do
+ note = <<~BODY
+ Single-line suggestion
+ ```suggestion
+ sug1
+ ```
+ BODY
+
+ expected = <<~BODY
+ Single-line suggestion
+ ```suggestion:-0+0
+ sug1
+ ```
+ BODY
+
+ expect(described_class.formatted_note_for(note: note)).to eq(expected)
+ end
+
+ it 'ignores text after suggestion tag on the same line' do
+ note = <<~BODY
+ looks like
+ ```suggestion text to be ignored
+ suggestion
+ ```
+ BODY
+
+ expected = <<~BODY
+ looks like
+ ```suggestion:-0+0
+ suggestion
+ ```
+ BODY
+
+ expect(described_class.formatted_note_for(note: note)).to eq(expected)
+ end
+
+ it 'formats multiple single-line suggestions' do
+ note = <<~BODY
+ Single-line suggestion
+ ```suggestion
+ sug1
+ ```
+ OR
+ ```suggestion
+ sug2
+ ```
+ BODY
+
+ expected = <<~BODY
+ Single-line suggestion
+ ```suggestion:-0+0
+ sug1
+ ```
+ OR
+ ```suggestion:-0+0
+ sug2
+ ```
+ BODY
+
+ expect(described_class.formatted_note_for(note: note)).to eq(expected)
+ end
+
+ it 'formats multi-line suggestions' do
+ note = <<~BODY
+ Multi-line suggestion
+ ```suggestion
+ sug1
+ ```
+ BODY
+
+ expected = <<~BODY
+ Multi-line suggestion
+ ```suggestion:-2+0
+ sug1
+ ```
+ BODY
+
+ expect(described_class.formatted_note_for(note: note, start_line: 6, end_line: 8)).to eq(expected)
+ end
+
+ it 'formats multiple multi-line suggestions' do
+ note = <<~BODY
+ Multi-line suggestion
+ ```suggestion
+ sug1
+ ```
+ OR
+ ```suggestion
+ sug2
+ ```
+ BODY
+
+ expected = <<~BODY
+ Multi-line suggestion
+ ```suggestion:-2+0
+ sug1
+ ```
+ OR
+ ```suggestion:-2+0
+ sug2
+ ```
+ BODY
+
+ expect(described_class.formatted_note_for(note: note, start_line: 6, end_line: 8)).to eq(expected)
+ end
+end
--
GitLab