Merge pull request #79 from wandersoncferreira/master

charignon · Oct 3, 2021 · 4d91dd6 · 4d91dd6
2 parents 341b7a1 + 34bfb7c
commit 4d91dd6
Show file tree

Hide file tree

Showing 3 changed files with 182 additions and 4 deletions.
diff --git a/README.md b/README.md
@@ -119,6 +119,14 @@ machine api.github.com login yourlogin^github-review password MYTOKENGOESHERE
 If you use GitHub Enterprise, you can use the `github-review-host` custom variable to
 configure the endpoint of your GitHub Enterprise installation, this should look like `api.git.mycompany.com`.
 
+- By default, `github-review` fetches only top level comments in a pull request.
+  You can set `github-review-view-comments-in-code-lines` to `t` to also fetch
+  comments made between code lines.
+
+  You can also enable comments between code lines that are outdated by setting
+  `github-review-view-comments-in-code-lines-outdated` to `t`, however we cannot
+  guarantee correct placement of these comments in the review buffer.
+
 ## Notice
 
 *I am providing code in the repository to you under an open source license. Because this is my personal repository, the license you receive to my

diff --git a/github-review.el b/github-review.el
@@ -60,6 +60,14 @@
   :group 'github-review
   :type 'string)
 
+(defcustom github-review-view-comments-in-code-lines nil
+  "Flag to enable displaying comments in code lines."
+  :group 'github-review)
+
+(defcustom github-review-view-comments-in-code-lines-outdated nil
+  "Flag to enable displaying outdated comments in code lines."
+  :group 'github-review)
+
 (defconst github-review-diffheader '(("Accept" . "application/vnd.github.v3.diff"))
   "Header for requesting diffs from GitHub.")
 
@@ -119,8 +127,26 @@ return a deferred object"
         nodes { author { login } bodyText state }
       } }
   }
+}" .repo .owner .num))
+          (query-with-comments (format "query {
+  repository(name: \"%s\", owner: \"%s\") {
+    pullRequest(number: %s){
+      headRef { target{ oid } }
+      title
+      bodyText
+      comments(first:50) {
+        nodes { author { login } bodyText }
+      }
+      reviews(first: 50) {
+        nodes { author { login } bodyText state
+          comments(first: 50)
+            { nodes { bodyText originalPosition position outdated path} }}
+      } }
+  }
 }" .repo .owner .num)))
-      (ghub-graphql query
+      (ghub-graphql (if github-review-view-comments-in-code-lines
+                        query-with-comments
+                      query)
                     '()
                     :auth 'github-review
                     :host (github-review-api-host pr-alist)
@@ -340,7 +366,82 @@ This function infers the PR name based on the current filename"
 (defun github-review-format-review (review)
   "Format a REVIEW object to string."
   (let-alist review
-    (format "Reviewed by @%s[%s]: %s" .author.login .state .bodyText)))
+    (if (not (string-empty-p .bodyText))
+        (format "Reviewed by @%s[%s]: %s" .author.login .state .bodyText)
+      "")))
+
+(defvar github-review-comment-pos ()
+  "Variable to count how many comments in code lines were added in the diff.
+This is necessary to adjust the new comments to the correct position in the diff given that
+Github API provides only the originalPosition in the query.")
+
+(defun github-review--get-how-many-comments-written (path)
+  (or (a-get github-review-comment-pos path) 0))
+
+(defun github-review-place-review-comments (gitdiff review)
+  (if (not (a-get-in review (list 'comments 'nodes)))
+      gitdiff
+    (let* ((at (a-get-in review (list 'author 'login)))
+           (body (a-get review 'bodyText))
+           (body-lines (split-string body "\n"))
+           (state (a-get review 'state))
+
+           (comments (a-get-in review (list 'comments 'nodes)))
+           (default-shift-pos 1))
+      (-reduce-from
+       (lambda (acc-diff comment)
+         (if (and (not github-review-view-comments-in-code-lines-outdated)
+                  (a-get comment 'outdated))
+             acc-diff
+           (let* ((path (a-get comment 'path))
+                  (original-pos (a-get comment 'originalPosition))
+                  (-position (a-get comment 'position))
+                  (position (when (numberp -position) -position))
+                  (adjusted-pos (+ (or position original-pos)
+                                   default-shift-pos
+                                   (github-review--get-how-many-comments-written path)))
+                  (comment-lines (split-string (a-get comment 'bodyText) "\n"))
+
+                  ;; get diff lines specific for the current path
+                  (gitdiff-path (s-concat "+++ b/" path "\n"))
+                  (gitdiff-splitted-on-path (split-string acc-diff gitdiff-path))
+                  (gitdiff-on-path-lines (split-string (-second-item gitdiff-splitted-on-path) "\n"))
+                  (gitdiff-on-path-splitted (-split-at adjusted-pos gitdiff-on-path-lines)))
+
+             ;; save how many lines of comments was written in the buffer for this path
+             (setf (alist-get path github-review-comment-pos nil nil 'equal)
+                   (+ (github-review--get-how-many-comments-written path)
+                      (length comment-lines)
+                      (if (string-empty-p body)
+                          0
+                        (length body-lines))))
+
+             ;; include comments on buffer for this path
+             (let* ((result
+                     (-concat
+                      (-first-item gitdiff-on-path-splitted)
+                      (list (format "~ Reviewed by @%s[%s]: %s" at state
+                                    (if (string-empty-p body)
+                                        (-first-item comment-lines)
+                                      (-first-item body-lines))))
+                      (-map
+                       (lambda (commentLine) (s-concat "~ " (s-trim-left commentLine)))
+                       (-concat
+                        (-drop 1 body-lines)
+                        (if (string-empty-p body)
+                            (-drop 1 comment-lines)
+                          comment-lines)))
+                      (-second-item gitdiff-on-path-splitted)))
+                    (gitdiff-on-path-new (s-concat
+                                          gitdiff-path
+                                          (s-join "\n" result))))
+
+               ;; join this path with beginning of the diff
+               (s-concat
+                (-first-item gitdiff-splitted-on-path)
+                gitdiff-on-path-new)))))
+       gitdiff
+       comments))))
 
 (defun github-review-format-diff (gitdiff pr)
   "Formats a GITDIFF and PR to save it for review."
@@ -366,7 +467,15 @@ This function infers the PR name based on the current filename"
                  #'github-review-to-comments
                  (-map #'github-review-format-review reviews)))
                "\n"))
-     (a-get gitdiff 'message))))
+     (if github-review-view-comments-in-code-lines
+         (progn
+           (setq github-review-comment-pos ())
+           (-reduce-from
+            (lambda (acc-gitdiff node)
+              (github-review-place-review-comments acc-gitdiff node))
+            (a-get gitdiff 'message)
+            .reviews.nodes))
+       (a-get gitdiff 'message)))))
 
 ;;;;;;;;;;;;;;;;;;;;;
 ;; User facing API ;;

diff --git a/test/github-review-test.el b/test/github-review-test.el
@@ -125,6 +125,37 @@ index 9eced0230..4512bb335 100644
 +type MultiBalanceReport    = PeriodicReport AccountName MixedAmount
 +type MultiBalanceReportRow = PeriodicReportRow AccountName MixedAmount
 
+ -- type alias just to remind us which AccountNames might be depth-clipped, below.
+ type ClippedAccountName = AccountName
+")
+
+    (defconst example-diff-before-comments-in-code-line "diff --git a/hledger-lib/Hledger/Reports/MultiBalanceReport.hs b/hledger-lib/Hledger/Reports/MultiBalanceReport.hs
+index 9eced0230..4512bb335 100644
+--- a/hledger-lib/Hledger/Reports/MultiBalanceReport.hs
++++ b/hledger-lib/Hledger/Reports/MultiBalanceReport.hs
+
+-type MultiBalanceReport    = PeriodicReport AccountLeaf MixedAmount
+-type MultiBalanceReportRow = PeriodicReportRow AccountLeaf MixedAmount
++type MultiBalanceReport    = PeriodicReport AccountName MixedAmount
++type MultiBalanceReportRow = PeriodicReportRow AccountName MixedAmount
+
+ -- type alias just to remind us which AccountNames might be depth-clipped, below.
+ type ClippedAccountName = AccountName
+")
+
+    (defconst example-diff-after-comments-in-code-line "diff --git a/hledger-lib/Hledger/Reports/MultiBalanceReport.hs b/hledger-lib/Hledger/Reports/MultiBalanceReport.hs
+index 9eced0230..4512bb335 100644
+--- a/hledger-lib/Hledger/Reports/MultiBalanceReport.hs
++++ b/hledger-lib/Hledger/Reports/MultiBalanceReport.hs
+
+-type MultiBalanceReport    = PeriodicReport AccountLeaf MixedAmount
+-type MultiBalanceReportRow = PeriodicReportRow AccountLeaf MixedAmount
+~ Reviewed by @babar[COMMENTED]: Very interesting change
+~ we should move forward
++type MultiBalanceReport    = PeriodicReport AccountName MixedAmount
++type MultiBalanceReportRow = PeriodicReportRow AccountName MixedAmount
+~ Reviewed by @babar[COMMENTED]: Change this code
+
  -- type alias just to remind us which AccountNames might be depth-clipped, below.
  type ClippedAccountName = AccountName
 ")
@@ -294,11 +325,41 @@ index 58baa4b..eae7707 100644
                                  'bodyText "LGTM"
                                  'state "APPROVED")))))
 
+    (defconst review-with-comments
+      (a-alist 'author (a-alist 'login "babar")
+               'state "COMMENTED"
+               'bodyText ""
+               'comments (a-alist
+                          'nodes
+                          (list (a-alist 'bodyText "Very interesting change\nwe should move forward"
+                                         'originalPosition 2
+                                         'outdated nil
+                                         'position ""
+                                         'path "hledger-lib/Hledger/Reports/MultiBalanceReport.hs")
+                                (a-alist 'bodyText "Change this code"
+                                         'originalPosition 4
+                                         'outdated nil
+                                         'position ""
+                                         'path "hledger-lib/Hledger/Reports/MultiBalanceReport.hs")))))
+
     (describe "github-review-format-diff"
       (it "can format a simple diff"
         (expect (a-equal (github-review-format-diff simple-diff simple-pr)simple-context-expected-review)))
       (it "can format a diff with top level comments and review"
-        (expect (a-equal (github-review-format-diff simple-diff pr-with-tl-comments)  expected-review-tl-comment)))))
+        (expect (a-equal (github-review-format-diff simple-diff pr-with-tl-comments)  expected-review-tl-comment))))
+
+    (describe "github-review-place-review-comments"
+      (before-all
+        (setq github-review-comment-pos ())
+        (setq github-review-view-comments-in-code-lines nil)
+        (setq github-review-view-comments-in-code-lines-outdated nil))
+      (it "can include PR comments made in code lines"
+        (expect (github-review-place-review-comments example-diff-before-comments-in-code-line review-with-comments)
+                :to-equal
+                example-diff-after-comments-in-code-line))
+      (it "`github-review-comment-pos' should have increased to 3 because we have 2 comments with 3 lines"
+        (expect github-review-comment-pos :to-equal '(("hledger-lib/Hledger/Reports/MultiBalanceReport.hs" . 3))))))
+
   (describe "entrypoints"
     (describe "github-review-start"
       :var (github-review-save-diff