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 diff --git a/github-review.el b/github-review.el index aecf221..e8c6379 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.") @@ -119,8 +123,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 databaseId } }} + } } + } }" .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) @@ -141,7 +163,7 @@ PR-ALIST is an alist representing a PR REVIEW is the review alist CALLBACK will be called back when done" (let-alist pr-alist - (ghub-post (format "/repos/%s/%s/pulls/%s/reviews" .owner .repo .num) + (ghub-post (format "/repos/%s/%s/pulls/%s/review" .owner .repo .num) nil :auth 'github-review :payload review @@ -149,6 +171,24 @@ CALLBACK will be called back when done" :errorback #'github-review-errback :callback callback))) +(defun github-review-post-review-replies (pr-alist replies callback) + "Submit replies to review comments inline." + (let-alist pr-alist + (-map + (lambda (comment) + (let* ((position (a-get comment 'position)) + (comment-id (a-get github-review-pos->databaseid position)) + (body (a-get comment 'body))) + (ghub-post (format "/repos/%s/%s/pulls/%s/comments/%s/replie" .owner .repo .num comment-id) + nil + :payload (a-alist 'body body) + :headers github-review-diffheader + :auth 'github-review + :host "api.github.com" + :callback (lambda (&rest _)) + :errorback #'github-review-errback))) + replies))) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; Code review file parsing ;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; @@ -223,7 +263,8 @@ ACC is an alist accumulating parsing state." (in-file? (not top-level?))) (cond ;; Previous comments are ignored and don't affect the parsing - ((github-review-previous-comment? l) acc) + ((github-review-previous-comment? l) + (a-assoc acc 'mark-previous-comment t)) ;; First cgithub-review-hunk ((and top-level? (github-review-hunk? l)) @@ -248,14 +289,17 @@ ACC is an alist accumulating parsing state." ;; For such comments we report it on on the first line (a-alist 'position (max .pos 1) 'path .path - 'body (github-review-comment-text l)) + 'body (github-review-comment-text l) + 'reply? .mark-previous-comment) .comments))) ;; Header before the filenames, restart the position ((github-review-is-start-of-file-hunk? l) (a-assoc acc 'pos nil)) ;; Any other line in a file - (in-file? (a-assoc acc 'pos (+ 1 .pos))) + (in-file? (a-assoc acc + 'pos (+ 1 .pos) + 'mark-previous-comment nil)) (t acc))))) @@ -264,6 +308,7 @@ ACC is an alist accumulating parsing state." (let* ((acc (a-alist 'path nil 'pos nil 'body "" + 'mark-previous-comment nil 'comments ())) (parsed-data (-reduce-from #'github-review-parse-line acc lines)) (parsed-comments (a-get parsed-data 'comments)) @@ -321,8 +366,26 @@ This function infers the PR name based on the current filename" (message "Submitting review, this may take a while ...") (let* ((pr-alist (github-review-pr-from-fname (buffer-file-name))) (parsed-review (github-review-parsed-review-from-current-buffer)) + (comments (a-get parsed-review 'comments)) + (regular-comments (-map + (lambda (c) + (a-dissoc c 'reply?)) + (-filter + (lambda (c) (not (a-get c 'reply?))) + comments))) + (reply-comments (-filter (lambda (c) (a-get c 'reply?)) + comments)) (head-sha (a-get pr-alist 'sha)) - (review (a-assoc parsed-review 'commit_id head-sha 'event kind))) + (review (a-assoc parsed-review + 'commit_id head-sha + 'event kind + 'comments regular-comments))) + (github-review-post-review-replies + pr-alist + reply-comments + (lambda (&rest _) + (message "Done submitting review replies"))) + (github-review-post-review pr-alist review (lambda (&rest _) @@ -340,7 +403,65 @@ 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-pos->databaseid ()) + +(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))) + 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 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 (a-get comment 'databaseId)) + github-review-pos->databaseid) + + (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)))) (defun github-review-format-diff (gitdiff pr) "Formats a GITDIFF and PR to save it for review." @@ -366,7 +487,16 @@ 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 nil) + (setq github-review-pos->databaseid ()) + (-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..fd0a451 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,34 @@ 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