Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Code review support #266

Closed
wants to merge 15 commits into from
Closed

Code review support #266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e0e2c57
Save more informations in some forge objects
JulienMasson Mar 16, 2020
830d1f9
gitlab: use discussions API for issue/pullreq posts
JulienMasson Mar 8, 2021
32fb522
gitlab: fetch pullreq versions
JulienMasson Mar 8, 2021
7ec4933
github: fill issue/pullreq/version objects
JulienMasson Mar 17, 2020
04f9931
pullreq: display version sections
JulienMasson Mar 17, 2020
8b06cb7
Refactor and don't display diff posts in topic view
JulienMasson Mar 17, 2020
aacb2ee
Display replies in thread section
JulienMasson Mar 17, 2020
795a4e2
pullreq: display diff posts
JulienMasson Mar 17, 2020
e77e9ff
Use forge-diff-visit-file in hunk section
JulienMasson Mar 17, 2020
cecd093
forge-create-diff-post: New command
JulienMasson Mar 17, 2020
a41f705
forge-reply-post: New command
JulienMasson Mar 17, 2020
49c72a8
gitlab: support pull single topic
JulienMasson Mar 17, 2020
cfa9e4d
Clean-up "comment" functions
JulienMasson Mar 17, 2020
b89bd57
github: handle diff post when deleting post
JulienMasson Mar 18, 2020
db7aeca
github: support edition of diff post
JulienMasson Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 44 additions & 6 deletions lisp/forge-commands.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,44 @@ point is currently on."
(insert (replace-regexp-in-string "^" "> " quote) "\n\n")))
(forge--display-post-buffer buf)))

(defun forge-create-diff-post ()
"Create a new diff post in the current `magit-diff' buffer."
(interactive)
(unless (derived-mode-p 'magit-diff-mode)
(user-error "This command is only available from magit-diff buffers"))
(let* ((file (magit-file-at-point))
(line (forge--pullreq-diff-current-line))
(topic forge-buffer-topic)
(version forge--pullreq-version)
(commit forge--pullreq-commit)
(header (format "New comment on %s, line %s - #%%i of %%p"
file line))
(buf (forge--prepare-post-buffer
(forge--format topic "%i:new-comment")
(forge--format topic header))))
(with-current-buffer buf
(setq forge--buffer-post-object topic)
(setq forge--buffer-post-args (list version commit file line))
(setq forge--submit-post-function 'forge--submit-create-diff-post))
(forge--display-post-buffer buf)))

;;; Reply

(defun forge-reply-post ()
"Reply to the current post."
(interactive)
(let* ((post (or (forge-post-at-point)
(user-error "There is no current post")))
(topic forge-buffer-topic)
(buf (forge--prepare-post-buffer
(forge--format post "%i:reply-comment")
(forge--format post "Reply comment on #%i of %p"))))
(with-current-buffer buf
(setq forge--buffer-post-object topic)
(setq forge--buffer-post-args (list post))
(setq forge--submit-post-function 'forge--submit-reply-post))
(forge--display-post-buffer buf)))

;;; Edit

(defun forge-edit-post ()
Expand Down Expand Up @@ -572,12 +610,12 @@ topic N and modify that instead."

;;; Delete

(defun forge-delete-comment (comment)
"Delete the comment at point."
(interactive (list (or (forge-comment-at-point)
(user-error "There is no comment at point"))))
(when (yes-or-no-p "Do you really want to delete the selected comment? ")
(forge--delete-comment (forge-get-repository t) comment)))
(defun forge-delete-post (post)
"Delete the post at point."
(interactive (list (or (forge-post-at-point)
(user-error "There is no post at point"))))
(when (yes-or-no-p "Do you really want to delete the selected post ? ")
(forge--delete-post (forge-get-repository t) post)))

;;; Branch

Expand Down
45 changes: 41 additions & 4 deletions lisp/forge-db.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
(defclass forge-database (closql-database)
((object-class :initform forge-repository)))

(defconst forge--db-version 7)
(defconst forge--db-version 8)
(defconst forge--sqlite-available-p
(with-demoted-errors "Forge initialization: %S"
(emacsql-sqlite-ensure-binary)
Expand Down Expand Up @@ -207,7 +207,9 @@
updated
body
(edits :default eieio-unbound)
(reactions :default eieio-unbound)]
(reactions :default eieio-unbound)
thread-id
reply-to]
(:foreign-key
[issue] :references issue [id]
:on-delete :cascade))
Expand Down Expand Up @@ -300,7 +302,8 @@
(reviews :default eieio-unbound)
(timeline :default eieio-unbound)
(marks :default eieio-unbound)
note]
note
(versions :default eieio-unbound)]
(:foreign-key
[repository] :references repository [id]
:on-delete :cascade))
Expand Down Expand Up @@ -342,7 +345,28 @@
updated
body
(edits :default eieio-unbound)
(reactions :default eieio-unbound)]
(reactions :default eieio-unbound)
thread-id
diff-p
resolved-by
reply-to
head-ref
commit-ref
base-ref
path
old-line
new-line]
(:foreign-key
[pullreq] :references pullreq [id]
:on-delete :cascade))

(pullreq-version
[(class :not-null)
(id :not-null :primary-key)
pullreq
number
head-ref
base-ref]
(:foreign-key
[pullreq] :references pullreq [id]
:on-delete :cascade))
Expand Down Expand Up @@ -429,6 +453,19 @@
;; Going forward create a backup before upgrading:
;; (message "Upgrading Forge database from version 7 to 8...")
;; (copy-file forge-database-file (concat forge-database-file "-v7"))
(when (= version 7)
(message "Upgrading Forge database from version 7 to 8...")
(copy-file forge-database-file (concat forge-database-file "-v7"))
(pcase-let ((`(,table . ,schema) (assq 'pullreq-version forge--db-table-schemata)))
(emacsql db [:create-table $i1 $S2] table schema))
(emacsql db [:alter-table pullreq :add-column versions :default $i1] eieio-unbound)
(dolist (c (list 'thread-id 'reply-to))
(emacsql db `[:alter-table issue-post :add-column ,c :default nil]))
(dolist (c (list 'thread-id 'diff-p 'resolved-by 'reply-to 'head-ref 'commit-ref
'base-ref 'path 'old-line 'new-line))
(emacsql db `[:alter-table pullreq-post :add-column ,c :default nil]))
(closql--db-set-version db (setq version 8))
(message "Upgrading Forge database from version 7 to 8...done"))
version))

;;; _
Expand Down
170 changes: 146 additions & 24 deletions lisp/forge-github.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@
(require 'forge-issue)
(require 'forge-pullreq)

;;; Variables

(defvar forge-github-headers
'(("Accept" . "application/vnd.github.comfort-fade-preview+json"))
"Custom Github Headers.")

;;; Class

(defclass forge-github-repository (forge-repository)
Expand Down Expand Up @@ -77,6 +83,7 @@
`((issues-until . ,(forge--topics-until repo until 'issue))
(pullRequests-until . ,(forge--topics-until repo until 'pullreq)))
:host (oref repo apihost)
:headers forge-github-headers
:auth 'forge)))

(cl-defmethod forge--pull-topic ((repo forge-github-repository) n
Expand All @@ -90,18 +97,25 @@
(oref repo name)
n
(lambda (data)
(funcall (if pullreqp #'forge--update-pullreq #'forge--update-issue)
repo data nil)
(with-current-buffer
(if (buffer-live-p buffer) buffer (current-buffer))
(magit-refresh)))
(let ((refresh-buffer
(lambda ()
(with-current-buffer
(if (buffer-live-p buffer) buffer (current-buffer))
(magit-refresh)))))
(if pullreqp
(progn
(forge--update-pullreq repo data nil)
(forge--ghub-fetch-review-threads repo n refresh-buffer))
(forge--update-issue repo data nil)
(funcall refresh-buffer))))
nil
:errorback
(and (not pullreqp)
(lambda (err _headers _status _req)
(when (equal (cdr (assq 'type (cadr err))) "NOT_FOUND")
(forge--pull-topic repo n t))))
:host (oref repo apihost)
:headers forge-github-headers
:auth 'forge)))

(cl-defmethod forge--update-repository ((repo forge-github-repository) data)
Expand Down Expand Up @@ -165,7 +179,8 @@
:author .author.login
:created .createdAt
:updated .updatedAt
:body (forge--sanitize-string .body))
:body (forge--sanitize-string .body)
:reply-to nil) ; cannot reply on these comments with github api
t)))
(when bump
(forge--set-id-slot repo issue 'assignees .assignees)
Expand All @@ -175,7 +190,8 @@

(cl-defmethod forge--update-pullreqs ((repo forge-github-repository) data bump)
(emacsql-with-transaction (forge-db)
(mapc (lambda (e) (forge--update-pullreq repo e bump)) data)))
(mapc (lambda (e) (forge--update-pullreq repo e bump)) data))
(forge--ghub-fetch-all-review-threads repo))

(cl-defmethod forge--update-pullreq ((repo forge-github-repository) data bump)
(emacsql-with-transaction (forge-db)
Expand All @@ -186,7 +202,9 @@
(forge-db)
(forge-pullreq :id pullreq-id
:repository (oref repo id)
:number .number)))))
:number .number))))
(head-ref .headRefOid)
(base-ref .baseRefOid))
(oset pullreq state (pcase-exhaustive .state
("MERGED" 'merged)
("CLOSED" 'closed)
Expand All @@ -213,19 +231,31 @@
.milestone.id)))
(oset pullreq body (forge--sanitize-string .body))
.databaseId ; Silence Emacs 25 byte-compiler.
(dolist (p .comments)
(let-alist p
;; Github API doesn't support pullreq versioning
;; Thus only add the latest version of the pullreq
(let* ((version-id (forge--object-id pullreq-id 1))
(version (forge-pullreq-version :id version-id
:pullreq pullreq-id
:number 1
:head-ref head-ref
:base-ref base-ref)))
(closql-insert (forge-db) version t))
;; add posts
(dolist (c .comments)
(let-alist c
(closql-insert
(forge-db)
(forge-pullreq-post
:id (forge--object-id pullreq-id .databaseId)
:pullreq pullreq-id
:number .databaseId
:author .author.login
:created .createdAt
:updated .updatedAt
:body (forge--sanitize-string .body))
t)))
:id (forge--object-id pullreq-id .databaseId)
:pullreq pullreq-id
:number .databaseId
:author .author.login
:created .createdAt
:updated .updatedAt
:body (forge--sanitize-string .body)
:diff-p nil
:reply-to nil) ;; cannot reply on these comments with github api
t)))
(when bump
(forge--set-id-slot repo pullreq 'assignees .assignees)
(forge--set-id-slot repo pullreq 'review-requests
Expand Down Expand Up @@ -473,6 +503,65 @@
(forge--pull repo nil cb))))
(funcall cb)))

(cl-defmethod forge--ghub-update-review-threads ((repo forge-github-repository) data)
(emacsql-with-transaction (forge-db)
(let-alist data
(let* ((pullreq-id (forge--object-id 'forge-pullreq repo .number))
(pullreq (forge-get-pullreq repo .number))
(head-ref .headRefOid)
(base-ref .baseRefOid))
;; add diff posts
(dolist (thread .reviewThreads)
(let-alist thread
(let ((new-line .line)
(old-line .originalLine)
(new (string= .diffSide "RIGHT")))
(dolist (c .comments)
(let-alist c
(closql-insert
(forge-db)
(forge-pullreq-post
:id (forge--object-id pullreq-id .databaseId)
:pullreq pullreq-id
:number .databaseId
:author .author.login
:created .createdAt
:updated .updatedAt
:body (forge--sanitize-string .body)
:diff-p t
:reply-to (when .replyTo .replyTo.databaseId)
:head-ref head-ref
:commit-ref (when .originalCommit .originalCommit.oid)
:base-ref base-ref
:path .path
:old-line (unless new old-line)
:new-line (when new (if old-line old-line new-line)))
t))))))
pullreq))))

(cl-defmethod forge--ghub-fetch-review-threads ((repo forge-github-repository) number
&optional callback)
(ghub-fetch-review-threads (oref repo owner)
(oref repo name)
number
(lambda (data)
(forge--ghub-update-review-threads repo data)
(when callback (funcall callback)))
nil
:host (oref repo apihost)
:headers forge-github-headers
:auth 'forge))

(cl-defmethod forge--ghub-fetch-all-review-threads ((repo forge-github-repository))
(when-let* ((pullreqs (oref repo pullreqs))
(pullreq (pop pullreqs))
(cb (lambda (pullreqs cb)
(when-let ((pullreq (pop pullreqs)))
(forge--ghub-fetch-review-threads repo (oref pullreq number)
(lambda () (funcall cb pullreqs cb)))))))
(forge--ghub-fetch-review-threads repo (oref pullreq number)
(lambda () (funcall cb pullreqs cb)))))

;;; Mutations

(cl-defmethod forge--create-pullreq-from-issue ((repo forge-github-repository)
Expand Down Expand Up @@ -536,12 +625,42 @@
:callback (forge--post-submit-callback)
:errorback (forge--post-submit-errorback)))

(cl-defmethod forge--submit-create-diff-post ((_ forge-github-repository) topic
&rest args)
(pcase-let ((`(,version ,commit ,file ,line) args))
(let* ((commit_id (if commit commit (oref version head-ref)))
(old-line (assoc-default 'old line))
(new-line (assoc-default 'new line))
(side (if new-line "RIGHT" "LEFT")))
(forge--ghub-post topic "/repos/:owner/:repo/pulls/:number/comments"
`((body . ,(string-trim (buffer-string)))
(commit_id . ,commit_id)
(path . ,file)
(line . ,(if new-line new-line old-line))
(side . ,side))
:headers forge-github-headers
:callback (forge--post-submit-callback)
:errorback (forge--post-submit-errorback)))))

(cl-defmethod forge--submit-reply-post ((_ forge-github-repository) topic
&rest args)
(let* ((reply-to (oref (car args) reply-to))
(id (if reply-to reply-to (oref (car args) number))))
(forge--ghub-post
topic
(format "/repos/:owner/:repo/pulls/:number/comments/%d/replies" id)
`((body . ,(string-trim (buffer-string))))
:callback (forge--post-submit-callback)
:errorback (forge--post-submit-errorback))))

(cl-defmethod forge--submit-edit-post ((_ forge-github-repository) post)
(forge--ghub-patch post
(cl-typecase post
(forge-pullreq "/repos/:owner/:repo/pulls/:number")
(forge-issue "/repos/:owner/:repo/issues/:number")
(forge-post "/repos/:owner/:repo/issues/comments/:number"))
(if (and (forge-pullreq-post-p post) (oref post diff-p))
"/repos/:owner/:repo/pulls/comments/:number"
(cl-typecase post
(forge-pullreq "/repos/:owner/:repo/pulls/:number")
(forge-issue "/repos/:owner/:repo/issues/:number")
(forge-post "/repos/:owner/:repo/issues/comments/:number")))
(if (cl-typep post 'forge-topic)
(let-alist (forge--topic-parse-buffer)
`((title . , .title)
Expand Down Expand Up @@ -585,9 +704,12 @@
:payload labels
:callback (forge--set-field-callback)))

(cl-defmethod forge--delete-comment
(cl-defmethod forge--delete-post
((_repo forge-github-repository) post)
(forge--ghub-delete post "/repos/:owner/:repo/issues/comments/:number")
(forge--ghub-delete post
(if (and (forge-pullreq-post-p post) (oref post diff-p))
"/repos/:owner/:repo/pulls/comments/:number"
"/repos/:owner/:repo/issues/comments/:number"))
(closql-delete post)
(magit-refresh))

Expand Down
Loading