diff --git a/README.md b/README.md index ccd9ac9..26446ab 100644 --- 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 index aecf221..5a9c9e3 100644 --- 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 index 457cf7c..78ed55f 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,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