From 446e061b5df366281e13022cffa50aac9b24d3d2 Mon Sep 17 00:00:00 2001 From: Laurent Charignon Date: Sun, 3 Mar 2019 21:28:21 -0800 Subject: [PATCH] #### Summary Before this PR when using `github-review` one can download PRs title, body and diff and insert top level and inline comments. After this PR, one can get more information when performing a review, including, reviews from other people, inline comments and issues top-level comments (made by replying to a PR). It also opens future work to support reply to inline comments (https://github.com/charignon/github-review/issues/9) It looks like this: ```diff ~ PR TITLE ~ ~ PR body ~ on multiple line ~ @foo: a comment at the top level ~ Reviewed by @babar[APPROVED]: LGTM (a review comment) ``` #### Progress This is really rough work in progress, docstrings aren't final and this isn't fully tested. - [X] Top level, issue comments - [X] Reviews state and summary - [ ] Inline comments (moved to another Change) #### Test Plan - [X] Add testing to start-review for new types of comments - [X] Test end to end with multiple scenarios This will enable adding new functionality like showing previous inline and top level comments. --- README.md | 3 + github-review.el | 225 +++++++++++++++++++++++------ test/github-review-test.el | 287 ++++++++++++++++++++++++++++--------- 3 files changed, 408 insertions(+), 107 deletions(-) diff --git a/README.md b/README.md index bef4384..9e860d5 100644 --- a/README.md +++ b/README.md @@ -97,3 +97,6 @@ machine api.github.com login yourlogin^github-review password MYTOKENGOESHERE If you use github entreprise, you can use the `github-review-host` custom variable to configure the endpoint of your github entreprise installation. + +By default `github-review` only fetches the PR title summary and diff. You can set `github-review-fetch-top-level-and-review-comments` to `t` to +enable fetching top level and review comments. diff --git a/github-review.el b/github-review.el index cf37718..dc49829 100644 --- a/github-review.el +++ b/github-review.el @@ -56,6 +56,11 @@ :group 'github-review :type 'string) +(defcustom github-review-fetch-top-level-and-review-comments nil + "If t, fetch the top level and review comments." + :group 'github-review + :type 'boolean) + (defconst github-review-diffheader '(("Accept" . "application/vnd.github.v3.diff")) "Header for requesting diffs from GitHub.") @@ -90,6 +95,22 @@ "Return an empty alist." '()) +(defconst github-review-url-scheme + '((get-pr . "/repos/%s/%s/pulls/%s") + (get-inline-comments . "/repos/%s/%s/pulls/%s/comments") + (get-review-comments . "/repos/%s/%s/pulls/%s/reviews") + (get-issue-comments . "/repos/%s/%s/issues/%s/comments") + (submit-review . "/repos/%s/%s/pulls/%s/reviews"))) + +(defun github-review-format-pr-url (kind pr-alist) + "Format a url for accessing the pr. +KIND is the kind of information to request. +PR-ALIST is an alist represenging the PR" + (format (github-review-a-get github-review-url-scheme kind) + (github-review-a-get pr-alist 'owner ) + (github-review-a-get pr-alist 'repo ) + (github-review-a-get pr-alist 'num ))) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; Communication with GitHub ;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; @@ -99,16 +120,14 @@ PR-ALIST is an alist representing a PR, NEEDS-DIFF t to return a diff nil to return the pr object CALLBACK to call back when done." - (ghub-get (format "/repos/%s/%s/pulls/%s" - (github-review-a-get pr-alist 'owner ) - (github-review-a-get pr-alist 'repo ) - (github-review-a-get pr-alist 'num )) nil - :unpaginate t - :headers (if needs-diff github-review-diffheader '()) - :auth 'github-review - :host github-review-host - :callback callback - :errorback (lambda (&rest _) (message "Error talking to GitHub")))) + (ghub-get (github-review-format-pr-url 'get-pr pr-alist) + nil + :unpaginate t + :headers (if needs-diff github-review-diffheader '()) + :auth 'github-review + :host github-review-host + :callback callback + :errorback (lambda (&rest _) (message "Error talking to GitHub")))) (defun github-review-get-pr-object (pr-alist callback) "Get a pr object given PR-ALIST an alist representing a PR. @@ -120,21 +139,87 @@ CALLBACK is called with the result" CALLBACK is called with the result" (github-review-get-pr pr-alist t callback)) - (defun github-review-post-review (pr-alist review callback) "Submit a code review. PR-ALIST is an alist representing a PR REVIEW is the review alist CALLBACK will be called back when done" - (ghub-post (format "/repos/%s/%s/pulls/%s/reviews" - (github-review-a-get pr-alist 'owner ) - (github-review-a-get pr-alist 'repo ) - (github-review-a-get pr-alist 'num )) nil - :auth 'github-review - :payload review - :host github-review-host - :errorback (lambda (&rest _) (message "Error talking to GitHub")) - :callback callback)) + (ghub-post (github-review-format-pr-url 'submit-review pr-alist) + nil + :auth 'github-review + :payload review + :host github-review-host + :errorback (lambda (&rest _) (message "Error talking to GitHub")) + :callback callback)) + +(defun github-review-get-inline-comments (pr-alist callback) + "Get the inline comments on a pr. +PR-ALIST is an alist representing a PR +CALLBACK will be called back when done" + (ghub-get (github-review-format-pr-url 'get-inline-comments pr-alist) + nil + :auth 'github-review + :host github-review-host + :errorback (lambda (&rest _) (message "Error talking to GitHub")) + :callback callback)) + +(defun github-review-get-reviews (pr-alist callback) + "Get the code reviews on a PR. +PR-ALIST is an alist representing a PR +CALLBACK will be called back when done" + (ghub-get (github-review-format-pr-url 'get-review-comments pr-alist) + nil + :auth 'github-review + :host github-review-host + :errorback (lambda (&rest _) (message "Error talking to GitHub")) + :callback callback)) + +(defun github-review-get-issue-comments (pr-alist callback) + "Get the top level comments on a PR. +PR-ALIST is an alist representing a PR +CALLBACK will be called back when done" + (ghub-get (github-review-format-pr-url 'get-issue-comments pr-alist) + nil + :auth 'github-review + :host github-review-host + :errorback (lambda (&rest _) (message "Error talking to GitHub")) + :callback callback)) + +(defun github-review-chain-call (arg ctx cb) + "Run one function of a chain of functions. +See `github-review-chain-calls' for more information. +ARG is passed as the first argument of every callback in the chain. +CB is called if the chain is done. +CTX is propagated between calls in the chain. +See github-review-chain-api-calls for more information" + (let ((chain (github-review-a-get ctx 'chain))) + (if (eq nil chain) + (funcall cb ctx) ;; Last function in the chain + (let* ((nextnode (car chain)) + (fn (github-review-a-get nextnode 'function)) + (fncallback (github-review-a-get nextnode 'callback)) + (key (github-review-a-get nextnode 'key)) + (newctx (github-review-a-assoc ctx 'chain (cdr chain)))) + (funcall fn arg + (lambda (v &rest _) + (github-review-chain-call + arg + (if key + (github-review-a-assoc newctx key v) + (funcall fncallback v newctx)) + cb))))))) + +(defun github-review-chain-calls (arg cb chain) + "Run a chain of functions. +CHAIN is a list of ALIST containing 'function and ('callback or 'key) +callback are called with ARG and a CTX (alist) in that order. +KEY can be used instead of callback if the result should be stored in key. +The result of the call to the callback should be the new CTX. +ARG is the first argument that every function in the chain will be taking +CB is the final callback to call with the resulting aggregate object + +For an example of how to use it, look at the tests" + (github-review-chain-call arg `((chain . ,chain)) cb)) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; Code review file parsing ;; @@ -189,7 +274,6 @@ NEW-COMMENT is a comment to consider" ;; Cannot merge the current comment with the last comment (t (github-review-a-assoc (github-review-a-assoc acc 'merged (cons lastcomment merged)) 'lastcomment new-comment))))) - (defun github-review-merge-comments (comments) "Takes COMMENTS, inline comments and return a merged list of comments. COMMENTS on the same file, same pos are coallesced" @@ -291,7 +375,6 @@ ACC is an alist accumulating parsing state." (github-review-a-assoc 'num (match-string 3 url))))) pr-alist)))) - (defun github-review-save-diff (pr-alist diff) "Save a DIFF (string) to a temp file named after pr specified by PR-ALIST." (find-file (format "%s/%s___%s___%s.diff" @@ -335,37 +418,99 @@ This function infers the PR name based on the current filename" "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)))) -(defun github-review-format-diff (diff title body) +(defun github-review-format-top-level-comment (com) + "Format a top level COM objectto string." + (let ((username (github-review-a-get (github-review-a-get com 'user) 'login)) + (body (github-review-a-get com 'body))) + (format "@%s: %s" username body))) + +(defun github-review-format-review (review) + "Format a REVIEW object to string." + (let ((username (github-review-a-get (github-review-a-get review 'user) 'login)) + (state (github-review-a-get review 'state)) + (body (github-review-a-get review 'body))) + (format "Reviewed by @%s[%s]: %s" username state body))) + +(defun github-review-format-diff (ctx) "Formats a diff to save it for review. -DIFF TITLE and BODY are strings" +CTX is the result of a callback chain to get information about a PR. +See ‘github-review-start’ for more information" + (let* ((ob (github-review-a-get ctx 'object)) + (title (github-review-a-get ob 'title)) + (body (github-review-a-get ob 'body)) + (top-level-comments (github-review-a-get ctx 'top-level-comments)) + (reviews (github-review-a-get ctx 'reviews)) + (diff (-> ctx (github-review-a-get 'diff) (github-review-a-get 'message)))) (concat - (github-review-to-comments title) "\n~" "\n" - (github-review-to-comments body) "\n" - diff)) + (github-review-to-comments title) + "\n~" + "\n" + (github-review-to-comments body) + "\n" + (when top-level-comments + (concat (s-join + "\n" + (-map + #'github-review-to-comments + (-map #'github-review-format-top-level-comment top-level-comments))) + "\n")) + (when reviews + (concat (s-join + "\n" + (-map + #'github-review-to-comments + (-map #'github-review-format-review reviews))) + "\n")) + diff))) ;;;;;;;;;;;;;;;;;;;;; ;; User facing API ;; ;;;;;;;;;;;;;;;;;;;;; +(defun github-review-chain () + "Return alist representing sequence of operations to get a PR's information. +Gets the PR diff, object, top level comments, and code reviews." + ;; Get the diff + `(((function . github-review-get-pr-diff) + (key . diff)) + + ;; ... the PR object + ((function . github-review-get-pr-object) + (callback . (lambda (v ctx &rest _) + (let* ((comms (github-review-a-get v 'comments)) + (review_comments (github-review-a-get v 'review_comments)) + (chain (github-review-a-get v 'chain))) + ;; And top level comments if there is any + (when (and github-review-fetch-top-level-and-review-comments + (> comms 0)) + (setq chain (cons `((function . github-review-get-issue-comments) + (key . top-level-comments)) + chain))) + ;; And code review if there is any + (when (and github-review-fetch-top-level-and-review-comments + (> review_comments 0)) + (setq chain (cons `((function . github-review-get-reviews) + (key . reviews)) + chain))) + (-> ctx + (github-review-a-assoc 'object v) + (github-review-a-assoc 'chain chain)))))))) + ;;;###autoload (defun github-review-start (url) "Start review given PR URL." (interactive "sPR URL: ") (let* ((pr-alist (github-review-pr-from-url url))) - (github-review-get-pr-diff + (github-review-chain-calls pr-alist - ;; Get the diff - (lambda (v &rest _) - (let ((diff (github-review-a-get v 'message))) - (github-review-get-pr-object - pr-alist - ;; Get the title and body - (lambda (v &rest _) - (let* ((body (github-review-a-get v 'body)) - (title (github-review-a-get v 'title)) - (txt (github-review-format-diff diff title body))) - ;; Write everything to a file - (github-review-save-diff pr-alist txt))))))))) + ;; Callback when done + (lambda (ctx) + (github-review-save-diff + pr-alist + (github-review-format-diff ctx))) + + (github-review-chain) + ))) ;;;###autoload (defun github-review-approve () diff --git a/test/github-review-test.el b/test/github-review-test.el index 233a56e..cf81dd3 100644 --- a/test/github-review-test.el +++ b/test/github-review-test.el @@ -2,12 +2,51 @@ (require 'github-review) (require 'buttercup) -(describe "github-review-hunk?" - (it "can correctly identify a hunk" - (expect (github-review-hunk? "foo") :to-be nil) - (expect (github-review-hunk? "@@ b/foo") :to-be t))) +(describe "github-review" + :var (ghub-get + ghub-post) + (before-each + (setf + ;; Prevent the tests from hitting the network + (symbol-function 'ghub-get) + (lambda (&rest _) + (error "Cannot make network call in tests")) + (symbol-function 'ghub-post) + (lambda (&rest _) + (error "Cannot make network call in tests")))) -(defconst example-review-deleted-file "# comment test + (defconst expected-review-tl-comment "~ title +~ in +~ three +~ lines +~ +~ body +~ part +~ @foo: a comment +~ Reviewed by @babar[APPROVED]: LGTM +a +b +c") + + (defconst simple-context-expected-review "~ title +~ in +~ three +~ lines +~ +~ body +~ part +a +b +c") + + (describe "diff parsing" + + (describe "github-review-hunk?" + (it "can correctly identify a hunk" + (expect (github-review-hunk? "foo") :to-be nil) + (expect (github-review-hunk? "@@ b/foo") :to-be t))) + + (defconst example-review-deleted-file "# comment test ~ remove bad ~ ~ @@ -24,14 +63,14 @@ index 8ad537d..0000000 - # Comment test") -(defconst expected-review-deleted-file - '((body . "comment test") - (comments - ((position . 5) - (body . "Comment test") - (path . "bar"))))) + (defconst expected-review-deleted-file + '((body . "comment test") + (comments + ((position . 5) + (body . "Comment test") + (path . "bar"))))) -(defconst examplediff "# This is a global comment at the top of the file + (defconst examplediff "# This is a global comment at the top of the file # with multiple # lines diff --git a/content/reference/google-closure-library.adoc b/content/reference/google-closure-library.adoc @@ -54,7 +93,7 @@ index 58baa4b..eae7707 100644 [[try-the-wrapper-libraries-first]] ") -(defconst example-previous-comments "~ Top level previous comment + (defconst example-previous-comments "~ Top level previous comment # This is a global comment at the top of the file # with multiple # lines @@ -80,7 +119,7 @@ index 58baa4b..eae7707 100644 [[try-the-wrapper-libraries-first]] ") -(defconst example-no-comment "# This is a global comment at the top of the file + (defconst example-no-comment "# This is a global comment at the top of the file # with multiple # lines diff --git a/content/reference/google-closure-library.adoc b/content/reference/google-closure-library.adoc @@ -97,56 +136,170 @@ index 58baa4b..eae7707 100644 [[try-the-wrapper-libraries-first]] ") -(defconst complex-review-expected - '((body . "This is a global comment at the top of the file\nwith multiple\nlines") - (comments - ((position . 6) - (body . "Some other comment inline") - (path . "content/reference/google-closure-library.adoc")) - ((position . 5) - (body . "And a comment inline about\na specific line\n```with some\ncode```") - (path . "content/reference/google-closure-library.adoc"))))) - -(describe "github-review-parse-review-lines" - (it "can parse a complex code review" - (let* ((actual (github-review-parse-review-lines (split-string examplediff "\n")))) - (expect actual :to-equal complex-review-expected))) - (it "can parse a code review with no comment" - (let* ((actual (github-review-parse-review-lines (split-string example-no-comment "\n"))) - (expected '((body . "This is a global comment at the top of the file\nwith multiple\nlines")))) - (expect actual :to-equal expected))) - (it "can parse a code review with deleted files" - (let* ((actual (github-review-parse-review-lines (split-string example-review-deleted-file "\n")))) - (expect actual :to-equal expected-review-deleted-file))) - (it "can parse a code review with previous comments but ignores it" - (let* ((actual (github-review-parse-review-lines (split-string example-previous-comments "\n")))) - (expect actual :to-equal complex-review-expected)))) - - -(describe "github-review-pr-from-fname" - (it "can parse fname and infer pr name" - (expect (github-review-pr-from-fname "/tmp/charignon___testgheapi___2.diff") :to-equal - '((num . "2") - (repo . "testgheapi") - (owner . "charignon"))))) - -(describe "github-review-pr-from-url" - (it "can parse url and infer pr details" - (expect (github-review-pr-from-url "https://github.com/charignon/testgheapi/pull/2") :to-equal - '((num . "2") - (repo . "testgheapi") - (owner . "charignon"))))) - -(describe "github-review-format-diff" - (it "can format a diff" - (expect (github-review-format-diff "a\nb\nc" "title\nin\nthree\nlines" "body\npart") :to-equal - "~ title -~ in -~ three -~ lines -~ -~ body -~ part -a -b -c"))) + (defconst complex-review-expected + '((body . "This is a global comment at the top of the file\nwith multiple\nlines") + (comments + ((position . 6) + (body . "Some other comment inline") + (path . "content/reference/google-closure-library.adoc")) + ((position . 5) + (body . "And a comment inline about\na specific line\n```with some\ncode```") + (path . "content/reference/google-closure-library.adoc"))))) + + (describe "github-review-parse-review-lines" + (it "can parse a complex code review" + (let* ((actual (github-review-parse-review-lines (split-string examplediff "\n")))) + (expect actual :to-equal complex-review-expected))) + (it "can parse a code review with no comment" + (let* ((actual (github-review-parse-review-lines (split-string example-no-comment "\n"))) + (expected '((body . "This is a global comment at the top of the file\nwith multiple\nlines")))) + (expect actual :to-equal expected))) + (it "can parse a code review with deleted files" + (let* ((actual (github-review-parse-review-lines (split-string example-review-deleted-file "\n")))) + (expect actual :to-equal expected-review-deleted-file))) + (it "can parse a code review with previous comments but ignores it" + (let* ((actual (github-review-parse-review-lines (split-string example-previous-comments "\n")))) + (expect actual :to-equal complex-review-expected))))) + + (describe "PR name inference from review filename and url" + + (describe "github-review-pr-from-fname" + (it "can parse fname and infer pr name" + (expect (github-review-pr-from-fname "/tmp/charignon___testgheapi___2.diff") :to-equal + '((num . "2") + (repo . "testgheapi") + (owner . "charignon"))))) + + (describe "github-review-pr-from-url" + (it "can parse url and infer pr details" + (expect (github-review-pr-from-url "https://github.com/charignon/testgheapi/pull/2") :to-equal + '((num . "2") + (repo . "testgheapi") + (owner . "charignon")))))) + + (describe "diff formatting" + + (defconst simple-context + `((object . ((title . "title\nin\nthree\nlines") + (body . "body\npart"))) + (diff . ((message . "a\nb\nc"))))) + + (defconst context-with-tl-comments + `((object . ((title . "title\nin\nthree\nlines") + (body . "body\npart"))) + (top-level-comments ((user . ((login . "foo"))) + (body . "a comment"))) + (reviews ((user . ((login . "babar"))) + (state . "APPROVED") + (body . "LGTM"))) + (diff . ((message . "a\nb\nc"))))) + + (describe "github-review-format-diff" + (it "can format a simple diff" + (expect (github-review-format-diff simple-context) + :to-equal simple-context-expected-review)) + (it "can format a diff with top level comments and review" + (expect (github-review-format-diff context-with-tl-comments) + :to-equal expected-review-tl-comment)))) + + (describe "callback logic" + + (describe "github-review-chain-calls" + (it "can execute a chain of one function" + (let ((res 8)) + (github-review-chain-calls + ;; No argument + nil + + ;; Final callback set res to the content of the 'foo key + (lambda (ctx) + (setq res (github-review-a-get ctx 'foo))) + + ;; Function simulate an api call, takes arg and callback + ;; and populate the 'foo key with the result + '(((function . (lambda (arg cb) (funcall cb 3))) + (key . foo)))) + (expect res :to-equal 3))) + (it "can execute a chain of two functions" + (let ((res 8)) + (github-review-chain-calls + ;; No argument + nil + + ;; Final callback set res to the content of the 'foo key + (lambda (ctx) + (setq res (github-review-a-get ctx 'foo))) + + '( + ;; Function simulate an api call, takes arg and callback + ;; and populate the 'foo key with the result + ((function . (lambda (arg cb) (funcall cb 3))) + (key . foo)) + ;; Function increment the content of the key foo + ((function . (lambda (arg cb) (funcall cb 42))) + (callback . (lambda (arg ctx) + (github-review-a-assoc ctx + 'foo + (+ 1 + (github-review-a-get ctx 'foo)))))))) + (expect res :to-equal 4))))) + + (describe "entrypoints" + (describe "github-review-start" + :var (github-review-save-diff + github-review-get-pr-obj + github-review-get-pr-diff + diff) + + (before-each + (setf + ;; Mock all the I/O for the test + (symbol-function 'github-review-save-diff) + (lambda (_ value) + (setq diff value)) + + (symbol-function 'github-review-get-pr-diff) + (lambda (_ cb) + (funcall cb `((message . "a\nb\nc")))) + + (symbol-function 'github-review-get-issue-comments) + (lambda (_ cb) + (funcall cb `(((user . ((login . "foo"))) + (body . "a comment"))))) + + (symbol-function 'github-review-get-reviews) + (lambda (_ cb) + (funcall cb `(((user . ((login . "babar"))) + (state . "APPROVED") + (body . "LGTM"))))))) + + (describe "no top level comments are present" + (before-each + (setf (symbol-function 'github-review-get-pr-object) + (lambda (_ cb) + (funcall cb + '((body . "body\npart") + (comments . 0) + (review_comments . 0) + (title . "title\nin\nthree\nlines")))))) + (it "can render a diff" + (github-review-start "https://github.com/charignon/github-review/pull/6") + (expect diff :to-equal simple-context-expected-review))) + + (describe "with top level comments" + (before-each + (setf (symbol-function 'github-review-get-pr-object) + (lambda (_ cb) + (funcall cb + '((body . "body\npart") + (comments . 1) + (review_comments . 1) + (title . "title\nin\nthree\nlines")))))) + (it "can render a diff" + (let ((github-review-fetch-top-level-and-review-comments t)) + (github-review-start "https://github.com/charignon/github-review/pull/6") + (expect diff :to-equal expected-review-tl-comment))))))) + +(provide 'github-review-test) + +;;; github-review-test.el ends here