Code review support #266
Code review support #266
Conversation
|
Thanks. I haven't tried it yet but I'm interested in review support. I see there's |
|
This looks very nice, thanks! |
Yes I think it's possible, I guess it would involve to define a new minor-mode like
No problem, thanks :) |
|
Hey, hope you don't mind me giving my 2¢. I'm working on something I want to comment on the data model proposed in the PR. I'm not familiar with GitLab as I haven't really used it, but from In GitHub Reviews are first-class resources that belong to a PR. Each Additionally this PR adds the concept of versions to pull-requests The data model I had in mind is GitHub-centric, doesn't overload the (pullreq-review
[(class :not-null)
(id :not-null :primary-key)
author
state
submitted
commit_id
pullreq
(body :default eieio-unbound)]
(:foreign-key
[pullreq] :references pullreq [id]
:on-delete :cascade))
(pullreq-review-comment
[(class :not-null)
(id :not-null :primary-key)
pullreq-review
author
body
in-reply-to
diff-hunk
path
commit_id
original_commit_id
start-line
original-start-line
line
original-line
side]
(:foreign-key
[pullreq-review] :references pullreq-review [id])
(:foreign-key
[in-reply-to :references pullreq-review-comment [id]]))Just in case I was thinking of using an org-mode buffer for the UI.
|
|
Hi @PuercoPop sorry for the late reply, I'm glad that you are also working on a solution :)
Yes that's correct, according to the Gitlab API, you don't have the "merge request approval" infos in the merge request discussion items (comments): These infos can be fetched here: However for the moment with the current version of
That's not only Gitlab concept, you also have versions concept with Gerrit, Phabricator ...
Your data model is also interesting. Also I feel like I didn't overload too much the forge objects in my PR.
Oh that sounds very interesting. In my PR I followed the current forge UI "spirit" which rely on |
|
Looks awesome. |
tarsius
force-pushed
the
master
branch
2 times, most recently
from
10fcab8
to
8beace1
Compare
|
Please guys, review and merge it, I wanted this feature for several years :) |
|
This PR looks mostly good. It depends on magit/ghub#110 and I am afraid that is merely a quick hack. It works fine as a proof-of-concept but it should at least be modified so that it does not change what However I don't think this is the right way forward. The problem is that we can no longer make a single request which then automatically turns into multiple requests in order to do unpaginate everything. We now must do multiple "root" requests and we can either do that in a hacky way by improving magit/ghub#110, or we can teach the code that does the automatic unpagination to "inject" additional requests that do not correspond to any data in the graphql response. Doing that would involve changing (massively undocumented and untested) If someone wants to move this forward they would have to experiment with that idea and be prepared for some strange and difficult to understand code that implements this part of |
JulienMasson
force-pushed
the
code-review-support
branch
from
1e39827
to
c4d0139
Compare
|
Hi @tarsius, I rebase my commits with upstream and made some changes. As you said in the previous message:
Yes that's correct. In that way all the data from a github repo are fetch in 2 steps:
I don't know if it's the best way to do that but that's a solution ... With this solution, I don't hit GitHub GraphQL API limitations: |
|
Thanks! I'll try to look at all this again this weekend. |
There was a problem hiding this comment.
(commit::) Clean-up "comment" functions
The initial description that is added when a topic is created is also a "post" but it isn't a "comment". These functions deal with "comments" so let's not change the names. Let's also keep the predicate that is not used internally but maybe externally.
| (forge--glab-get repo | ||
| (format "/projects/%s/issues/%s/notes" .project_id .iid) | ||
| (format "/projects/%s/issues/%s/discussions" .project_id .iid) |
There was a problem hiding this comment.
Are "notes" and "discussions" the same thing and the latter is just the new name? If so, then please use a separate commit and talk about that.
There was a problem hiding this comment.
(commit::) gitlab: fill objects with discussions/merge-requests api
Additionally a lot of refactoring seems to be going on. That seems unnecessary -- can you add the new data without the factoring? If so, then please do; else split the refactoring and the adding of new data into two commits.
There was a problem hiding this comment.
(commit::) github: support edition of diff post
(commit::) github: handle diff post when deleting post
Are these bug fixes that don't really have much to do with the other changes in this pr? If so, then I would like to apply them master quickly, before we finish up this pr.
|
Turns out I don't know how to review a single commit. The above reviews ended up being attached to "the overall state as of now" so I had to add references to the appropriate commits manually. 😞 |
tarsius
force-pushed
the
code-review-support
branch
from
b2d3219
to
09e7b1c
Compare
|
Please edit this an similar comments like so: - ;; insert topic description
+ ;; Insert topic description. |
lisp/forge-topic.el
Outdated
| (forge--insert-section author created body 'forge-post-heading))) | ||
| ;; insert posts related to the topic | ||
| (dolist (post posts) | ||
| (unless (when (forge-pullreq-p topic) (oref post diff-p)) |
There was a problem hiding this comment.
In such cases that are about the returned value, as opposed to some side-effect, you should always use and instead of when. This unless on the other hand is "correct" because that is about the side-effect of its body.
| (or (and (fboundp 'forge-pullreq-p) | ||
| (forge-pullreq-p topic)) | ||
| (oref repo selective-p))) | ||
| (if topic |
| @@ -205,8 +205,7 @@ This variable has to be customized before `forge' is loaded." | |||
| (defun forge--topic-string-to-number (s) | |||
| (save-match-data | |||
| (if (string-match "\\`\\([!#]\\)?\\([0-9]+\\)" s) | |||
| (* (if (equal (match-string 1 s) "!") -1 1) | |||
| (string-to-number (match-string 2 s))) | |||
| (string-to-number (match-string 2 s)) | |||
There was a problem hiding this comment.
well no, I don't really understand why we need to return a negative value when it match "!" (gitlab PR).
If we want to pull single topic, with a negative value it failed.
I have already applied this to Which reminds me, please rebase onto |
|
This seems like a great feature. It would be cool if this PR could get wrapped up at some point so that all Melpa users can benefit from it. |
JulienMasson
force-pushed
the
code-review-support
branch
from
09e7b1c
to
f11d959
Compare
|
sorry for the very late reply. @tarsius, I have just rebased the PR onto I need to re-check all the tests listed in "Features Tested" table. For now, I played a little bit on Github/Gitlab projects with some Pull Requests / Issues. I was in contact with Github support and It seems that they fix the issue 6.:
I am able now to add comments on removed section :) For gitlab, I still have the issue described here: |
|
thank you @JulienMasson, your effort is |
tarsius
force-pushed
the
code-review-support
branch
from
f11d959
to
d3fa686
Compare
Turns out there was a reason why I passed a cursor ( (let* ((cur '(((a . A))))
(elt (car cur)))
(setf (alist-get 'b elt) 'B)
cur)
=> (((a . A)))
(let ((cur '(((a . A)))))
(setf (alist-get 'b (car cur)) 'B)
cur)
=> (((b . B)
(a . A)))I have removed the above commit. |
JulienMasson
force-pushed
the
code-review-support
branch
from
f11d959
to
05617dd
Compare
Yes I've been faced to the issue also, since you re-write the history I've rebased my commits on top of the new head. Following your last comments, there is 3 new/updated commits: |
JulienMasson
force-pushed
the
code-review-support
branch
from
05617dd
to
0946242
Compare
|
Guys please merge this PR I've been stalking this PR for weeks now lol. |
In order to improve user experience and support new features like code-review or replies, some informations need to be added in issue / pullrequest objects and a new object pullreq-version has been created. * issue-post object: -> thread-id: id of a thread discussion -> reply-to: id of the initial post in reply to * pullreq object: -> versions: list of version object * pullreq-post object: -> thread-id: id of a thread discussion -> diff-p: describe if this post is a diff post -> resolved-by: the user who resolved this thread -> reply-to: id of the initial post in reply to -> head-ref: head SHA for this post -> commit-ref: commit SHA for this post -> base-ref: base SHA for this post -> path: file path where the diff is posted -> old-line: the line where the diff is posted before changes -> new-line: the line where the diff is posted after changes * pullreq-version: contains informations like the base and head SHA reference of a pullreq version. These informations are needed for eg. when we want to know for which version of a pullreq are diff posts. If an old forge database is detected, it will be upgraded to the new version 8. Signed-off-by: Julien Masson <[email protected]>
tarsius
force-pushed
the
code-review-support
branch
from
0946242
to
09e8a61
Compare
The discussions API provide more informations for each issue/pullreq post. We can use this API to retrieve the following informations: - Issue: thread-id, reply-to - Pullreq: thread-id, diff-p, reply-to, head/commit/base ref, path, old-line, new-line Source: https://docs.gitlab.com/ee/api/discussions.html Signed-off-by: Julien Masson <[email protected]>
The gitlab API give the possibility to fetch versions for each diff from a pullreq. Source: https://docs.gitlab.com/ee/api/merge_requests.html#get-mr-diff-versions Signed-off-by: Julien Masson <[email protected]>
With PullRequest and PullRequestReviewThread graphql object, we can set some fields of issue, pull-request and version objects. In order to access to some elements of PullRequestReviewThread, we need to toggle "Multi-Line Comments Preview": https://developer.github.com/v4/previews/#multi-line-comments-preview Source: https://developer.github.com/v4/object/pullrequest/ https://developer.github.com/v4/object/pullrequestreviewthread/ Signed-off-by: Julien Masson <[email protected]>
In the pullreq view, the version sections are now displayed instead of commits. These sections contains commits and diff changes for each version. The number of comments per commits or diff changes are also displayed. Example: Diff changes --> Latest Version: (6 comments) Commits --> 3634ab3 commit-A (4 comments) Commits --> 0df0d75 commit-B (1 comments) Commits --> 441de84 commit-C (1 comments) Press RET on "Diff changes" will show the diff of all the commits. Press RET on "Commits" will show the corresponding commit. Signed-off-by: Julien Masson <[email protected]>
In the topic view the diff posts should not be displayed. We only display posts/replies related to the topic conversation. Signed-off-by: Julien Masson <[email protected]>
The replies are now inserted in the correct thread section and are indented. Signed-off-by: Julien Masson <[email protected]>
After RET in version section, the diff posts are now displayed in the magit diff buffer. Signed-off-by: Julien Masson <[email protected]>
Originally when pressing RET in a hunk section, it opens the file at the right position in the buffer. With a magit-diff buffer which contains diff posts, this feature is broken, the position in the file buffer is not correct. This patch handle this case when the magit-diff buffer contains diff posts. Signed-off-by: Julien Masson <[email protected]>
This new command allow the user to create a diff post from a magit-diff buffer. The diff buffer has to be generated from a pullreq. This command is only supported by Gitlab and Github. Signed-off-by: Julien Masson <[email protected]>
This new command allow the user to reply to any post. This command is only supported by Gitlab and Github. Signed-off-by: Julien Masson <[email protected]>
With a gitlab repo, it's now possible to pull a single topic. It can be either a pullreq or issue topic. Signed-off-by: Julien Masson <[email protected]>
In the code all the "comment" are treated as a post object. A post object can be found in a pull-request or an issue for eg. To avoid confusion it makes more sense to use "post" instead of "comment" in the function name when handling post object. Also forge-comment-at-point function has been removed since it's not used. Signed-off-by: Julien Masson <[email protected]>
Signed-off-by: Julien Masson <[email protected]>
When editing a diff post, the URL must be the following format: /repos/:owner/:repo/pulls/comments/:comment_id Source: https://developer.github.com/v3/pulls/comments/#edit-a-comment Signed-off-by: Julien Masson <[email protected]>
tarsius
force-pushed
the
code-review-support
branch
from
09e8a61
to
db7aeca
Compare
|
I just pushed some minor fixes and some cosmetic changes. diff --git a/lisp/forge-github.el b/lisp/forge-github.el
index a4b9c658..6ed93697 100644
--- a/lisp/forge-github.el
+++ b/lisp/forge-github.el
@@ -98,10 +98,10 @@ (cl-defmethod forge--pull-topic ((repo forge-github-repository) n
n
(lambda (data)
(let ((refresh-buffer
- (lambda () (with-current-buffer (if (buffer-live-p buffer)
- buffer
- (current-buffer))
- (magit-refresh)))))
+ (lambda ()
+ (with-current-buffer
+ (if (buffer-live-p buffer) buffer (current-buffer))
+ (magit-refresh)))))
(if pullreqp
(progn
(forge--update-pullreq repo data nil)
diff --git a/lisp/forge-gitlab.el b/lisp/forge-gitlab.el
index a6dedb99..74aefb9f 100644
--- a/lisp/forge-gitlab.el
+++ b/lisp/forge-gitlab.el
@@ -93,6 +93,7 @@ (cl-defmethod forge--pull-topic ((repo forge-gitlab-repository) n)
(with-current-buffer ,orig-buffer
(magit-refresh))))))
(if (forge-issue-p forge-buffer-topic)
+ ;; FIXME forge--fetch-issue does not exist.
(forge--fetch-issue repo (forge-get-issue n)
(cb #'forge--update-issue orig-buffer))
(forge--fetch-pullreq repo (forge-get-pullreq repo n)
@@ -199,9 +200,9 @@ (cl-defmethod forge--update-issue ((repo forge-gitlab-repository) data)
(forge--set-id-slot repo issue 'labels .labels))
.body .id ; Silence Emacs 25 byte-compiler.
(dolist (d .posts)
- (let* ((thread-id (cdr (assq 'id d)))
- (notes (cdr (assq 'notes d)))
- (reply-to (cdr (assq 'id (car notes)))))
+ (let* ((notes (cdr (assq 'notes d)))
+ (reply-to (cdr (assq 'id (car notes))))
+ (thread-id (cdr (assq 'id d))))
(dolist (c notes)
(let-alist c
(let ((post
@@ -214,8 +215,8 @@ (cl-defmethod forge--update-issue ((repo forge-gitlab-repository) data)
:updated .updated_at
:body (forge--sanitize-string .body)
:thread-id thread-id
- :reply-to (unless (zerop (cl-position c notes))
- reply-to))))
+ :reply-to (and (not (zerop (cl-position c notes)))
+ reply-to))))
(closql-insert (forge-db) post t))))))))))
;;;; Pullreqs
@@ -266,19 +267,14 @@ (cl-defmethod forge--fetch-pullreq ((repo forge-gitlab-repository) pullreq callb
(if done
(funcall callback repo (car cur))
(cond
- ;; fetch source project
((not (assq 'source_project (car cur)))
(forge--fetch-pullreq-source-repo repo cur cb))
- ;; fetch target project
((not (assq 'target_project (car cur)))
(forge--fetch-pullreq-target-repo repo cur cb))
- ;; fetch posts
((not (assq 'posts (car cur)))
(forge--fetch-pullreq-posts repo cur cb))
- ;; fetch versions
((not (assq 'versions (car cur)))
(forge--fetch-pullreq-versions repo cur cb))
- ;; done
(t (setq done t)
(funcall cb cb cur))))))))
(forge--glab-get repo (format "/projects/:project/merge_requests/%d"
@@ -379,7 +375,6 @@ (cl-defmethod forge--update-pullreq ((repo forge-gitlab-repository) data)
(forge--set-id-slot repo pullreq 'assignees (list .assignee))
(forge--set-id-slot repo pullreq 'labels .labels))
.body .id ; Silence Emacs 25 byte-compiler.
- ;; add diff versions
(dolist (v .versions)
(let-alist v
(let* ((version-id (forge--object-id pullreq-id .id))
@@ -390,11 +385,10 @@ (cl-defmethod forge--update-pullreq ((repo forge-gitlab-repository) data)
:head-ref .head_commit_sha
:base-ref .base_commit_sha)))
(closql-insert (forge-db) version t))))
- ;; add posts
(dolist (d .posts)
- (let* ((thread-id (cdr (assq 'id d)))
- (notes (cdr (assq 'notes d)))
- (reply-to (cdr (assq 'id (car notes)))))
+ (let* ((notes (cdr (assq 'notes d)))
+ (reply-to (cdr (assq 'id (car notes))))
+ (thread-id (cdr (assq 'id d))))
(dolist (c notes)
(let-alist c
(let ((post
@@ -408,14 +402,14 @@ (cl-defmethod forge--update-pullreq ((repo forge-gitlab-repository) data)
:body (forge--sanitize-string .body)
:thread-id thread-id
:diff-p (string= "DiffNote" .type)
- :reply-to (unless (zerop (cl-position c notes))
- reply-to)
- :head-ref (when .position .position.head_sha)
- :commit-ref (when .position .position.start_sha)
- :base-ref (when .position .position.base_sha)
- :path (when .position .position.new_path)
- :old-line (when .position .position.old_line)
- :new-line (when .position .position.new_line))))
+ :reply-to (and (not (zerop (cl-position c notes)))
+ reply-to)
+ :head-ref (and .position .position.head_sha)
+ :commit-ref (and .position .position.start_sha)
+ :base-ref (and .position .position.base_sha)
+ :path (and .position .position.new_path)
+ :old-line (and .position .position.old_line)
+ :new-line (and .position .position.new_line))))
(closql-insert (forge-db) post t))))))))))
;;;; Other
diff --git a/lisp/forge-pullreq.el b/lisp/forge-pullreq.el
index 55af66be..bbbf93e1 100644
--- a/lisp/forge-pullreq.el
+++ b/lisp/forge-pullreq.el
@@ -357,7 +357,8 @@ (defun forge-diff-visit-file (file)
(assoc-default 'new line))))
(with-current-buffer (magit-diff-visit-file--internal
file nil #'switch-to-buffer-other-window)
- (goto-line file-line)
+ (goto-char (point-min))
+ (forward-line (1- file-line))
(move-to-column column)))
(magit-diff-visit-file file)))
@@ -372,15 +373,16 @@ (defun forge--pullreq-diff-get-line (file line goto-from)
(cur-line start))
(save-excursion
(goto-char content)
- (while (and (not (= cur-line line))
- (not (eobp)))
+ (while (not (or (= cur-line line)
+ (eobp)))
(unless (or (magit-section-value-if 'post)
- (string-match-p (if goto-from "\\+" "-")
- (buffer-substring (point) (+ (point) 1))))
+ (string-match-p
+ (if goto-from "\\+" "-")
+ (buffer-substring (point) (1+ (point)))))
(cl-incf cur-line))
(forward-line))
- (unless (= (point) (point-max))
- (line-number-at-pos (point)))))))))
+ (and (not (= (point) (point-max)))
+ (line-number-at-pos (point)))))))))
(defun forge--pullreq-diff-current-line ()
(when-let ((hunk-section (magit-diff-visit--hunk)))
@@ -412,24 +414,22 @@ (defun forge--insert-pullreq-diff-posts (diff-posts)
(let* ((inhibit-read-only t)
(root-section magit-root-section))
(dolist (post diff-posts)
- (with-slots (reply-to path old-line new-line number author created body) post
+ (with-slots (reply-to path old-line new-line number author created body)
+ post
(unless reply-to
(save-excursion
- ;; goto to the line in diff
- (when-let ((line (if new-line
- (forge--pullreq-diff-get-line path new-line nil)
- (forge--pullreq-diff-get-line path old-line t))))
- (goto-line (+ line 1))
+ (when-let ((line (forge--pullreq-diff-get-line
+ path old-line new-line)))
+ (goto-char (point-min))
+ (forward-line line)
(while (magit-section-value-if 'post) (forward-line))
- ;; insert post
(magit-insert-section section (post post)
- (oset section heading-highlight-face 'forge-pullreq-diff-post-heading)
+ (oset section heading-highlight-face
+ 'forge-pullreq-diff-post-heading)
(forge--insert-section author created body
'forge-pullreq-diff-post-heading)
- ;; insert replies of this post
(forge--insert-replies diff-posts number
'forge-pullreq-diff-post-reply-heading)
- ;; insert delimitation line
(overlay-put (make-overlay (- (point) 1) (point))
'face 'forge-pullreq-diff-delimitation)))
(setq magit-root-section root-section)))))))
@@ -463,7 +463,8 @@ (defun forge-show-pullreq-diff ()
(setq forge--pullreq-commit commit)
(setq forge--pullreq-buffer pullreq-buffer)
(setq forge-buffer-topic topic)
- (add-hook 'magit-unwind-refresh-hook 'forge--pullreq-diff-refresh nil t)
+ (add-hook 'magit-unwind-refresh-hook
+ 'forge--pullreq-diff-refresh nil t)
(magit-refresh)))))
(defun forge--insert-pullreq-diff-commits (version diff-commits diff-posts)
@@ -473,11 +474,12 @@ (defun forge--insert-pullreq-diff-commits (version diff-commits diff-posts)
(let ((posts (forge--filter-diff-posts-by-commit diff-posts id)))
(insert (concat (propertize abbrev-id 'face 'magit-hash)
(format " %-50s " subject)
- (when posts
- (propertize (format "(%d comments)" (length posts))
- 'face 'magit-section-heading)))
- "\n")))))
- (insert "\n"))
+ (and posts
+ (propertize
+ (format "(%d comments)" (length posts))
+ 'face 'magit-section-heading))
+ "\n"))))))
+ (insert "\n"))
(defun forge--insert-pullreq-versions (pullreq)
(let ((posts (oref pullreq posts))
@@ -486,17 +488,19 @@ (defun forge--insert-pullreq-versions (pullreq)
(dolist (version versions)
(with-slots (base-ref head-ref) version
(cl-incf count)
- (let* ((diff-commits (magit-git-lines "log" "--format=(\"%H\" \"%h\" \"%s\")"
- (format "%s..%s" base-ref head-ref)))
+ (let* ((diff-commits (magit-git-lines
+ "log" "--format=(\"%H\" \"%h\" \"%s\")"
+ (format "%s..%s" base-ref head-ref)))
(diff-posts (forge--filter-diff-posts-by-version posts version))
(comments-nbr (format "(%d comments) " (length diff-posts)))
(hide (not (eq count (length versions)))))
- ;; all the version section are collapsed except for the latest version
+ ;; All the version section are collapsed except for the
+ ;; latest version.
(magit-insert-section (pullreq-diff (list version) hide)
(magit-insert-heading (concat (if (= count (length versions))
"Latest Version: "
(format "Version %d: " count))
- (when diff-posts comments-nbr)))
+ (and diff-posts comments-nbr)))
(forge--insert-pullreq-diff-commits version
(mapcar #'read diff-commits)
diff-posts)))))))
diff --git a/lisp/forge-topic.el b/lisp/forge-topic.el
index 4bc2b9a5..62efb227 100644
--- a/lisp/forge-topic.el
+++ b/lisp/forge-topic.el
@@ -435,18 +435,19 @@ (defun forge--filter-reply-posts (posts id)
(cl-remove-if-not (lambda (post)
(and (oref post reply-to)
(= (oref post reply-to) id)))
- posts))
+ posts))
(defun forge--insert-replies (posts id face)
(when-let ((replies (forge--filter-reply-posts posts id))
(indent-column 4))
(dolist (reply replies)
(with-slots (author created body) reply
- (magit-insert-section section (post reply)
- (let* ((author (concat (make-string indent-column
- (string-to-char " "))
- author))
- (heading (forge--section-heading author created)))
+ (magit-insert-section (post reply)
+ (let ((heading (forge--section-heading
+ (concat (make-string indent-column
+ (string-to-char " "))
+ author)
+ created)))
(add-face-text-property indent-column (length heading)
face t heading)
(magit-insert-heading heading)
@@ -462,7 +463,6 @@ (defun forge-topic-refresh-buffer ()
(format "%s: %s" forge-buffer-topic-ident (oref topic title)))
(magit-insert-section (topicbuf)
(magit-insert-headers 'forge-topic-headers-hook)
- ;; insert pullreq versions
(when (forge-pullreq-p topic)
(forge--insert-pullreq-versions topic))
(when-let ((note (oref topic note)))
|
|
I have finally decided to not merge this after all. Until now I was planning to "review one more time" and then merge, but I have to be honest with @JulienMasson, interested users and myself: I have done the "one last look" a few times now and I am just not getting closer to actually merging this. The quality of this pull-request is good. Julien, you clearly understood the existing code and what you were doing. You've also listened to my feedback and have been very patient. The effort that went into this from on your side is probably why it took me so long to finally reject this contribution. (For the future I would suggest that you discuss such large and laborious contributions with the maintainer before hand. In this case I would most likely have told you that the prospects of me merging something were not good.) I am not sure I want to add this functionality at all. I can see the appeal, but at the same time I avoid using this feature even when using the Github and Gitlab web interfaces. I find it confusing but don't think the general idea is misguided. I do however suspect that these two implementations are local maxima and that means I am skeptical of a more or less direct port. I find this a very interesting topic and I would like to slowly build up something, likely over the course of multiple years. At the same time maintain an implementation and trying to preserve backward compatibility would hinder experimentation drastically. Please keep in mind, that the original plan was to create an Emacs tool that can work with multiple issue-tracking "back-ends" such as forges (github/gitlab/...), mailing lists and others. Ultimately I imagined the most advanced back-end to be one that is not tied to any external platform or protocol but one that stores all data in the repository. Alas this has not happened because the decision to support forges first in order to have something that is immediately useful, proofed to be very useful indeed but also to be a lot of work just by itself. It was so much work that I did not find any time to re-imagine a tool for collaboration from scratch. I fear that merging this pull-request would tie Forge even more closely to these platforms and their design decision, and so I have no choice but to decline this contribution. I hope you can all understand my decision. -- Jonas Ps: @JulienMasson maybe you could maintain this functionality in a fork for others to use. But I would certainly understand if you chose not to. |
|
Hi @tarsius, I understand your point, no problem. Now for the future of this, I am not really motivated to maintain a fork. Thanks |
I'm glad.
That would be even better of course. But yeah; not trivial to pull of. |
|
This is closed but for people that come here looking for this feature, please see |
Prelude:
First thanks a lot for the all work you have done around magit, forge, transient ...
They are parts of the modules that I use the most and it saves me a LOT of times :)
I started to use Forge several months ago, there is a lot of features implemented, it works really great.
However there is one feature missing that I'm interested is to be able to do a Code-Review.
I've seen there is an issue already opened on this subject #75 but it seems that you don't have time to work on this feature at the moment.
So I thought it can be interesting to help/contribute on that.
At the beginning I was only interested to add the support for Gitlab.
I had to do a lot of changes, more than I expected and I started to think that I might break some Github features.
Thus I continued and added the support for Github.
After that I had the same feeling that I might break "Issue" features since there is some common code with pullreq (they are all topic), so I did also some changes around "Issue" code.
As a result I finished with this Pull-Request which contains a lot of changes.
I tried to follow the design of forge source code and not introduced some complexity with my understanding of the code.
I'm not an expert of Emacs so I might implemented wrongly some features.
This Pull-Request is really a "Proof of concept".
I don't know if you already have some design in mind on this subject.
I'm open to any discussion, code refactor, UI changes ...
You will find below screenshots on Github/Gitlab and a table of the features tested.
I also listed some known issues that I've been faced during my tests.
Most of them are (according to me) BUGS from Github/Gitlab API.
Warning:
For Github there is a dependency with the following pull-request on Ghub poject:
magit/ghub#110
Screenshots:
Github:
Gitlab:
Features Tested:
Known issues:
Github: pullreq: need to single pull manually to have diff posts
Code review support ghub#110
Github: reply to "normal" post not supported by the API
Github: versioning not supported by the API, only display last version
Gitlab: commit reference incorrect for some diff post
I opened an issue on Gitlab for that:
https://gitlab.com/gitlab-org/gitlab/-/issues/211775
Gitlab: cannot create diff post on specific commit (server internal error)
Github: cannot create diff post in removed section
I sent a message to Github Support on this issue, no answer for the moment ...
Github: posts removed by Github Web still present in database
Comments placed out of diff context are not seen
Unsupported features: