From c04cee3cbc4235641902a7b7677fb8e5b919dbdb Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Fri, 1 Oct 2021 20:45:51 -0300 Subject: [PATCH 01/17] first working version --- github-review.el | 76 ++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 9 deletions(-) diff --git a/github-review.el b/github-review.el index aecf221..09809fb 100644 --- a/github-review.el +++ b/github-review.el @@ -116,7 +116,8 @@ return a deferred object" nodes { author { login } bodyText } } reviews(first: 50) { - nodes { author { login } bodyText state } + nodes { author { login } bodyText state + comments(first: 50) { nodes { bodyText diffHunk originalPosition } }} } } } }" .repo .owner .num))) @@ -342,9 +343,59 @@ This function infers the PR name based on the current filename" (let-alist review (format "Reviewed by @%s[%s]: %s" .author.login .state .bodyText))) +(setq github-review-comment-pos ()) + +(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)) + + (diffs (split-string gitdiff "\n")) + (comments (a-get-in review (list 'comments 'nodes))) + (default-shift-pos 5) + (diffs-new + (-reduce-from + (lambda (acc-diff comment) + (let* ((original-pos (a-get comment 'originalPosition)) + (adjusted-pos (+ original-pos + default-shift-pos + (or (a-get github-review-comment-pos original-pos) 0))) + (comment-lines (split-string (a-get comment 'bodyText) "\n")) + (diff-splitted (-split-at adjusted-pos acc-diff))) + + (push (cons original-pos (+ (or (a-get github-review-comment-pos original-pos) 0) + (length comment-lines) + (length body-lines))) + github-review-comment-pos) + (-concat + (-first-item diff-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 diff-splitted)))) + diffs + comments))) + (s-join + "\n" + diffs-new)))) + (defun github-review-format-diff (gitdiff pr) "Formats a GITDIFF and PR to save it for review." + (message "AQUI sim") (let-alist pr + (setq github-review-comment-pos ()) (concat (github-review-to-comments .title) "\n~" @@ -359,14 +410,20 @@ This function infers the PR name based on the current filename" #'github-review-to-comments (-map #'github-review-format-top-level-comment .comments.nodes))) "\n")) - (when-let ((reviews (-reject (lambda (x) (string= (a-get x 'body) "")) .reviews.nodes))) - (concat (s-join - "\n" - (-map - #'github-review-to-comments - (-map #'github-review-format-review reviews))) - "\n")) - (a-get gitdiff 'message)))) + (-reduce-from + (lambda (acc-gitdiff node) + (github-review-place-review-comments acc-gitdiff node)) + (a-get gitdiff 'message) + .reviews.nodes) + ;; (when-let ((reviews (-reject (lambda (x) (string= (a-get x 'body) "")) .reviews.nodes))) + ;; (concat (s-join + ;; "\n" + ;; (-map + ;; #'github-review-to-comments + ;; (-map #'github-review-format-review reviews))) + ;; "\n")) + ;; (a-get gitdiff 'message) + ))) ;;;;;;;;;;;;;;;;;;;;; ;; User facing API ;; @@ -382,6 +439,7 @@ This function infers the PR name based on the current filename" (deferred:nextc it (lambda (x) (let-alist (-second-item x) + (github-review-save-diff (a-assoc pr-alist 'sha .data.repository.pullRequest.headRef.target.oid) (github-review-format-diff (-first-item x) .data.repository.pullRequest))))) From 500b44af5edc1d8df3012b89494a53c8a100cf58 Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Fri, 1 Oct 2021 21:04:40 -0300 Subject: [PATCH 02/17] fixed bug in positioning --- github-review.el | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/github-review.el b/github-review.el index 09809fb..43469c6 100644 --- a/github-review.el +++ b/github-review.el @@ -343,7 +343,7 @@ This function infers the PR name based on the current filename" (let-alist review (format "Reviewed by @%s[%s]: %s" .author.login .state .bodyText))) -(setq github-review-comment-pos ()) +(setq github-review-comment-pos nil) (defun github-review-place-review-comments (gitdiff review) (if (not (a-get-in review (list 'comments 'nodes))) @@ -362,14 +362,13 @@ This function infers the PR name based on the current filename" (let* ((original-pos (a-get comment 'originalPosition)) (adjusted-pos (+ original-pos default-shift-pos - (or (a-get github-review-comment-pos original-pos) 0))) + (or github-review-comment-pos 0))) (comment-lines (split-string (a-get comment 'bodyText) "\n")) (diff-splitted (-split-at adjusted-pos acc-diff))) - (push (cons original-pos (+ (or (a-get github-review-comment-pos original-pos) 0) - (length comment-lines) - (length body-lines))) - github-review-comment-pos) + (setq github-review-comment-pos (+ (or github-review-comment-pos 0) + (length comment-lines) + (length body-lines))) (-concat (-first-item diff-splitted) (list "" @@ -393,9 +392,8 @@ This function infers the PR name based on the current filename" (defun github-review-format-diff (gitdiff pr) "Formats a GITDIFF and PR to save it for review." - (message "AQUI sim") (let-alist pr - (setq github-review-comment-pos ()) + (setq github-review-comment-pos nil) (concat (github-review-to-comments .title) "\n~" From 2c1e03051182d493cd5019c0be087077a8b2e022 Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Fri, 1 Oct 2021 21:10:38 -0300 Subject: [PATCH 03/17] include general comments --- github-review.el | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/github-review.el b/github-review.el index 43469c6..8e72516 100644 --- a/github-review.el +++ b/github-review.el @@ -341,7 +341,9 @@ 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) + ""))) (setq github-review-comment-pos nil) @@ -408,6 +410,15 @@ This function infers the PR name based on the current filename" #'github-review-to-comments (-map #'github-review-format-top-level-comment .comments.nodes))) "\n")) + "\n" + (when-let ((reviews (-reject (lambda (x) (string= (a-get x 'body) "")) .reviews.nodes))) + (concat (s-join + "\n" + (-map + #'github-review-to-comments + (-map #'github-review-format-review reviews))) + "\n")) + "\n" (-reduce-from (lambda (acc-gitdiff node) (github-review-place-review-comments acc-gitdiff node)) From 7aa87df60d0f31d68e6d43ff5e6953e0547ce01c Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Fri, 1 Oct 2021 21:19:47 -0300 Subject: [PATCH 04/17] improve clarity --- github-review.el | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/github-review.el b/github-review.el index 8e72516..bcff4db 100644 --- a/github-review.el +++ b/github-review.el @@ -331,12 +331,12 @@ This function infers the PR name based on the current filename" (defun github-review-to-comments (text) "Convert TEXT, a string to a string where each line is prefixed by ~." - (s-join "\n" (-map (lambda (x) (concat "~ " x)) (s-split "\n" text)))) + (s-join "\n" (-concat (list " ") (-map (lambda (x) (concat "~ " x)) (s-split "\n" text))))) (defun github-review-format-top-level-comment (com) "Format a top level COM objectto string." (let-alist com - (format "@%s: %s" .author.login .bodyText))) + (format "Commented by @%s: %s" .author.login .bodyText))) (defun github-review-format-review (review) "Format a REVIEW object to string." From b062d7d23dc2ce7f845e60d286e2557d63eee22a Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Fri, 1 Oct 2021 21:20:41 -0300 Subject: [PATCH 05/17] remove comments --- github-review.el | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/github-review.el b/github-review.el index bcff4db..b95d0be 100644 --- a/github-review.el +++ b/github-review.el @@ -423,16 +423,7 @@ This function infers the PR name based on the current filename" (lambda (acc-gitdiff node) (github-review-place-review-comments acc-gitdiff node)) (a-get gitdiff 'message) - .reviews.nodes) - ;; (when-let ((reviews (-reject (lambda (x) (string= (a-get x 'body) "")) .reviews.nodes))) - ;; (concat (s-join - ;; "\n" - ;; (-map - ;; #'github-review-to-comments - ;; (-map #'github-review-format-review reviews))) - ;; "\n")) - ;; (a-get gitdiff 'message) - ))) + .reviews.nodes)))) ;;;;;;;;;;;;;;;;;;;;; ;; User facing API ;; From 4ee9ed32f82a862561237002fe827974d0340010 Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Sat, 2 Oct 2021 08:34:22 -0300 Subject: [PATCH 06/17] remove unused diffHunk from graphql query --- github-review.el | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/github-review.el b/github-review.el index b95d0be..417f110 100644 --- a/github-review.el +++ b/github-review.el @@ -117,7 +117,8 @@ return a deferred object" } reviews(first: 50) { nodes { author { login } bodyText state - comments(first: 50) { nodes { bodyText diffHunk originalPosition } }} + comments(first: 50) + { nodes { bodyText originalPosition } }} } } } }" .repo .owner .num))) From e9a32e33165347c2b54e4036cf2d06e6dafce6d4 Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Sat, 2 Oct 2021 08:34:37 -0300 Subject: [PATCH 07/17] guard feature behind flag --- github-review.el | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/github-review.el b/github-review.el index 417f110..2deb170 100644 --- a/github-review.el +++ b/github-review.el @@ -60,6 +60,10 @@ :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) + (defconst github-review-diffheader '(("Accept" . "application/vnd.github.v3.diff")) "Header for requesting diffs from GitHub.") @@ -419,12 +423,13 @@ This function infers the PR name based on the current filename" #'github-review-to-comments (-map #'github-review-format-review reviews))) "\n")) - "\n" - (-reduce-from - (lambda (acc-gitdiff node) - (github-review-place-review-comments acc-gitdiff node)) - (a-get gitdiff 'message) - .reviews.nodes)))) + (if github-review-view-comments-in-code-lines + (-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 ;; From 7ed15f0a0adf9bd95a4f1d8b5197374d4afb3dac Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Sat, 2 Oct 2021 08:35:37 -0300 Subject: [PATCH 08/17] unecessary newline --- github-review.el | 1 - 1 file changed, 1 deletion(-) diff --git a/github-review.el b/github-review.el index 2deb170..53d1cae 100644 --- a/github-review.el +++ b/github-review.el @@ -415,7 +415,6 @@ This function infers the PR name based on the current filename" #'github-review-to-comments (-map #'github-review-format-top-level-comment .comments.nodes))) "\n")) - "\n" (when-let ((reviews (-reject (lambda (x) (string= (a-get x 'body) "")) .reviews.nodes))) (concat (s-join "\n" From e4cc0147df63075ed4264447879443b9bef0d761 Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Sat, 2 Oct 2021 08:35:53 -0300 Subject: [PATCH 09/17] remove unecessary new line --- github-review.el | 1 - 1 file changed, 1 deletion(-) diff --git a/github-review.el b/github-review.el index 53d1cae..d4bd32a 100644 --- a/github-review.el +++ b/github-review.el @@ -444,7 +444,6 @@ This function infers the PR name based on the current filename" (deferred:nextc it (lambda (x) (let-alist (-second-item x) - (github-review-save-diff (a-assoc pr-alist 'sha .data.repository.pullRequest.headRef.target.oid) (github-review-format-diff (-first-item x) .data.repository.pullRequest))))) From e227389a765f43d4ca47c6efb8a2675070217fc9 Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Sat, 2 Oct 2021 08:38:31 -0300 Subject: [PATCH 10/17] docs to var --- github-review.el | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/github-review.el b/github-review.el index d4bd32a..05b5cba 100644 --- a/github-review.el +++ b/github-review.el @@ -350,7 +350,10 @@ This function infers the PR name based on the current filename" (format "Reviewed by @%s[%s]: %s" .author.login .state .bodyText) ""))) -(setq github-review-comment-pos nil) +(defvar github-review-comment-pos nil + "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-place-review-comments (gitdiff review) (if (not (a-get-in review (list 'comments 'nodes))) From bf94ed005a9fed8142eff336d7cc1f24df0a44c3 Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Sat, 2 Oct 2021 13:33:05 -0300 Subject: [PATCH 11/17] guard new query behind flag --- github-review.el | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/github-review.el b/github-review.el index 05b5cba..c751a8e 100644 --- a/github-review.el +++ b/github-review.el @@ -111,6 +111,20 @@ return a deferred object" (defun github-review-get-pr-info (pr-alist callback) (let-alist pr-alist (let ((query (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 } + } } + } +}" .repo .owner .num)) + (query-with-comments (format "query { repository(name: \"%s\", owner: \"%s\") { pullRequest(number: %s){ headRef { target{ oid } } @@ -126,7 +140,9 @@ return a deferred object" } } } }" .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) From 74c0ec1674b8e6e8979b7be0e4d22872b1cf2583 Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Sat, 2 Oct 2021 14:05:11 -0300 Subject: [PATCH 12/17] readme instructions --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index ccd9ac9..4ba3279 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,10 @@ 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. + ## 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 From 395359e7e865b5de06c62154e97f1216fcc5d198 Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Sat, 2 Oct 2021 19:39:39 -0300 Subject: [PATCH 13/17] move setq to guarded branch --- github-review.el | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/github-review.el b/github-review.el index c751a8e..3a237d9 100644 --- a/github-review.el +++ b/github-review.el @@ -419,7 +419,6 @@ Github API provides only the originalPosition in the query.") (defun github-review-format-diff (gitdiff pr) "Formats a GITDIFF and PR to save it for review." (let-alist pr - (setq github-review-comment-pos nil) (concat (github-review-to-comments .title) "\n~" @@ -442,11 +441,13 @@ Github API provides only the originalPosition in the query.") (-map #'github-review-format-review reviews))) "\n")) (if github-review-view-comments-in-code-lines - (-reduce-from - (lambda (acc-gitdiff node) - (github-review-place-review-comments acc-gitdiff node)) - (a-get gitdiff 'message) - .reviews.nodes) + (progn + (setq github-review-comment-pos nil) + (-reduce-from + (lambda (acc-gitdiff node) + (github-review-place-review-comments acc-gitdiff node)) + (a-get gitdiff 'message) + .reviews.nodes)) (a-get gitdiff 'message))))) ;;;;;;;;;;;;;;;;;;;;; From 6c2d092be9d0884a002bfe022a3a85797d6cb474 Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Sat, 2 Oct 2021 22:10:20 -0300 Subject: [PATCH 14/17] test case and cleanup to not break previous tests --- github-review.el | 11 ++++---- test/github-review-test.el | 57 +++++++++++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/github-review.el b/github-review.el index 3a237d9..54cc5f0 100644 --- a/github-review.el +++ b/github-review.el @@ -352,12 +352,12 @@ This function infers the PR name based on the current filename" (defun github-review-to-comments (text) "Convert TEXT, a string to a string where each line is prefixed by ~." - (s-join "\n" (-concat (list " ") (-map (lambda (x) (concat "~ " x)) (s-split "\n" text))))) + (s-join "\n" (-map (lambda (x) (concat "~ " x)) (s-split "\n" text)))) (defun github-review-format-top-level-comment (com) "Format a top level COM objectto string." (let-alist com - (format "Commented by @%s: %s" .author.login .bodyText))) + (format "@%s: %s" .author.login .bodyText))) (defun github-review-format-review (review) "Format a REVIEW object to string." @@ -394,11 +394,12 @@ Github API provides only the originalPosition in the query.") (setq github-review-comment-pos (+ (or github-review-comment-pos 0) (length comment-lines) - (length body-lines))) + (if (string-empty-p body) + 0 + (length body-lines)))) (-concat (-first-item diff-splitted) - (list "" - (format "~ Reviewed by @%s[%s]: %s" at state + (list (format "~ Reviewed by @%s[%s]: %s" at state (if (string-empty-p body) (-first-item comment-lines) (-first-item body-lines)))) diff --git a/test/github-review-test.el b/test/github-review-test.el index 457cf7c..6b41ea0 100644 --- 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,35 @@ 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) + (a-alist 'bodyText "Change this code" + 'originalPosition 4) + )))) + (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 nil) + (setq github-review-view-comments-in-code-lines 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 3)))) + (describe "entrypoints" (describe "github-review-start" :var (github-review-save-diff From 99d71fd153d00805e420a2455ca719b92b1f4dba Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Sat, 2 Oct 2021 22:12:37 -0300 Subject: [PATCH 15/17] fix alignment --- test/github-review-test.el | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/github-review-test.el b/test/github-review-test.el index 6b41ea0..fd0a451 100644 --- a/test/github-review-test.el +++ b/test/github-review-test.el @@ -333,9 +333,8 @@ index 58baa4b..eae7707 100644 'nodes (list (a-alist 'bodyText "Very interesting change\nwe should move forward" 'originalPosition 2) - (a-alist 'bodyText "Change this code" - 'originalPosition 4) - )))) + (a-alist 'bodyText "Change this code" + 'originalPosition 4))))) (describe "github-review-format-diff" (it "can format a simple diff" From d336d8460f01a0a84855024b91b9cba228dfea10 Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Sun, 3 Oct 2021 10:54:51 -0300 Subject: [PATCH 16/17] fix misconcept on pos field, it's path dependent --- github-review.el | 104 +++++++++++++++++++++++-------------- test/github-review-test.el | 17 ++++-- 2 files changed, 77 insertions(+), 44 deletions(-) diff --git a/github-review.el b/github-review.el index 54cc5f0..5a9c9e3 100644 --- a/github-review.el +++ b/github-review.el @@ -64,6 +64,10 @@ "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.") @@ -136,7 +140,7 @@ return a deferred object" reviews(first: 50) { nodes { author { login } bodyText state comments(first: 50) - { nodes { bodyText originalPosition } }} + { nodes { bodyText originalPosition position outdated path} }} } } } }" .repo .owner .num))) @@ -366,11 +370,14 @@ This function infers the PR name based on the current filename" (format "Reviewed by @%s[%s]: %s" .author.login .state .bodyText) ""))) -(defvar github-review-comment-pos nil +(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 @@ -379,43 +386,62 @@ Github API provides only the originalPosition in the query.") (body-lines (split-string body "\n")) (state (a-get review 'state)) - (diffs (split-string gitdiff "\n")) (comments (a-get-in review (list 'comments 'nodes))) - (default-shift-pos 5) - (diffs-new - (-reduce-from - (lambda (acc-diff comment) - (let* ((original-pos (a-get comment 'originalPosition)) - (adjusted-pos (+ original-pos - default-shift-pos - (or github-review-comment-pos 0))) - (comment-lines (split-string (a-get comment 'bodyText) "\n")) - (diff-splitted (-split-at adjusted-pos acc-diff))) - - (setq github-review-comment-pos (+ (or github-review-comment-pos 0) - (length comment-lines) - (if (string-empty-p body) - 0 - (length body-lines)))) - (-concat - (-first-item diff-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 diff-splitted)))) - diffs - comments))) - (s-join - "\n" - diffs-new)))) + (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." @@ -443,7 +469,7 @@ Github API provides only the originalPosition in the query.") "\n")) (if github-review-view-comments-in-code-lines (progn - (setq github-review-comment-pos nil) + (setq github-review-comment-pos ()) (-reduce-from (lambda (acc-gitdiff node) (github-review-place-review-comments acc-gitdiff node)) diff --git a/test/github-review-test.el b/test/github-review-test.el index fd0a451..78ed55f 100644 --- a/test/github-review-test.el +++ b/test/github-review-test.el @@ -332,9 +332,15 @@ index 58baa4b..eae7707 100644 'comments (a-alist 'nodes (list (a-alist 'bodyText "Very interesting change\nwe should move forward" - 'originalPosition 2) + 'originalPosition 2 + 'outdated nil + 'position "" + 'path "hledger-lib/Hledger/Reports/MultiBalanceReport.hs") (a-alist 'bodyText "Change this code" - 'originalPosition 4))))) + 'originalPosition 4 + 'outdated nil + 'position "" + 'path "hledger-lib/Hledger/Reports/MultiBalanceReport.hs"))))) (describe "github-review-format-diff" (it "can format a simple diff" @@ -344,14 +350,15 @@ index 58baa4b..eae7707 100644 (describe "github-review-place-review-comments" (before-all - (setq github-review-comment-pos nil) - (setq github-review-view-comments-in-code-lines nil)) + (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 3)))) + (expect github-review-comment-pos :to-equal '(("hledger-lib/Hledger/Reports/MultiBalanceReport.hs" . 3)))))) (describe "entrypoints" (describe "github-review-start" From 34bfb7cccd4f7bbcfce7c15cd940e60190ed0817 Mon Sep 17 00:00:00 2001 From: Wanderson Ferreira Date: Sun, 3 Oct 2021 10:57:38 -0300 Subject: [PATCH 17/17] improve docs on README about outdated --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 4ba3279..26446ab 100644 --- a/README.md +++ b/README.md @@ -123,6 +123,10 @@ configure the endpoint of your GitHub Enterprise installation, this should look 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