[ Feature Request ] Comments Editing
ldelossa opened this issue · 10 comments
It would be pretty useful to be able to view, edit, and create comments on a given pull request.
I began looking into this feature and the hub command line allows for listing open PRs and listing comments within those PRs. The command returns JSON.
I'm curious if vim-fugitive maybe interested in this feature and if so work to get it moving. I am pretty novice in vim-script but, with a plan, I'm willing to do the necessary work as it would be pretty awesome to do code reviews right from Vim.
The major difference I see with this feature is API requirement and taking the hub command on as a dependency. A assume a polling mechanism would need to exist as well to get the latest comments at some interval.
Let me know what you guys think.
Fugitive is absolutely not interested in this issue, but Rhubarb could be. I imagine the JSON API already in place can largely do the same job that Hub can without the dependency.
@tpope sorry, didn't realize the issue was transferred.
Does vim-rhubarb already have a method for polling the api for updates?
The way to do this is to make a special URL buffer, presumably similar to a fugitive://... buffer but with rhurbab://, that loads the issue and a list of comments, and perhaps further URLs for each individual comment. Calling :edit in the buffer reloads, which could optionally be accompanied by a map like nnoremap <buffer> r :edit<CR>.
I'm not sure polling is a good idea (you really plan to leave an issue open in a window and watch for updates?), but even if it's the best idea in the universe, I won't entertain further discussion of it until the basics are in place.
@tpope
In this feature request I'm focusing more on comments during a pull-request not comments in issues.
The Github API requires you to provide a pull request number and responds with all comments in that pull request. JSON objects have both file name details and hunk details.
If the feature is to support code reviews within Vim - then I had originally imaged some interactivity in the process, when new PR comments are created, they should show up in Vim. That being said - I'll defer to you for the "best way" to go about that - I am new to Vim scripting.
With the focus being on pull request would you comments on the URL buffer still stand?
I will also need to discover "remotes" and call the GitHub API with the appropriate remote to list the correct pull requests in flight.
I was thinking of plain comments - the same kind that appear on issues - and hadn't really taken diff comments into account. This sounds way more useful but it's also way less obvious to me how it should work. How should it work?
I'll dump my initial brain storming - there will definitely be gaps to fill.
A high level vim command is registered, lets call it 'GitCommentsToggle' for sake of example.
When triggered we can parse the output of git remote show origin (lets ignore other remotes for now) and grab the owner/repo combination for API usage.
We can make an API call to list "in flight" pull requests and then introduce some form of user input to "select" the PR we want to load comments for.
API call is made to grab all the comments for this PR. Comment objects have some key fields (from an example pr comment on one of my projects):
"diff_hunk": "@@ -47,25 +45,19 @@ func Test_Matcher_Integration(t *testing.T) {\n \tctx, cancel := context.WithTimeout(ctx, 2*time.Minute)\n \tdefer cancel()\n \tup.Update(ctx)\n-\n \tpath := filepath.Join(\"testdata\", \"indexreport-buster-jackson-databind.json\")\n \tf, err := os.Open(path)\n \tif err != nil {\n \t\tt.Fatalf(\"%v\", err)\n \t}\n \n-\tvar sr claircore.IndexReport\n-\terr = json.NewDecoder(f).Decode(&sr)\n+\tvar ir claircore.IndexReport",
"path": "debian/matcher_integration_test.go",
"position": 42,
"original_position": 42,
"body": "renamed to ir",
"created_at": "2019-12-28T02:35:24Z",
"updated_at": "2019-12-28T02:35:25Z",
With this path information and line number we can then find if the buffer for that file is open and create some character either in the gutter or some indicator like a linter would to indicate a comment is at that line. Then, in my head, a floating window could display the comment but that's a vim version/implementation detail.
That would get us displaying the comments. Posting comments would entail placing your cursor in a buffer at a particular line number, opening a buffer where your comment body can go, and then making a POST api call with details similar to the response posted above.
Step 3 implies we're on the branch for a particular PR. Step 1 makes me pick a PR from a list of all open PRs. There is one correct PR and if we pick any other PR we get invalid results. Please ... think this through a bit more.
Even past that easily rectifiable oversight I don't understand your workflow. If you're reviewing a PR, you're not looking at random files, you're looking at a diff and maybe diving into a file to get additional context. If you're resolving a reviewed PR, you're presumably starting with the comments and want to jump to the files they reference. Neither of these would be particularly helped by splattering a bunch of signs and floating windows over the files you already have open.
Good catch on the PR mismatch. I can work out a better flow for that which removes the ambiguity.
As far as workflow however...
I would argue the opposite. That is how you perform a code review in the GitHub UI, you are scoped to particular set of chunks, and have to dig further to get the full file view... and if you for some reason want to browse a file outside of the diffs... well you go open VIM.
One of the reasons to approach code review in Vim is to have a Vim native experience. Opening non-related files should be supported, and viewing comments should be out of the way until you desire them. I'd like to avoid a scenario with Vim is adapted heavily for the purpose (like the diff UI in vim-fugitive).
For example, I have a PR that changes some interface. This interface changes may affect files which are not diff'd in the PR. If code review was happening solely in Vim, I could still use git diff to find what changed, go to that block, see any comments for that hunk, and now also jump to any other files outside the PR to confirm the changes I'm looking at are correct.
I would argue the opposite. That is how you perform a code review in the GitHub UI, you are scoped to particular set of chunks, and have to dig further to get the full file view... and if you for some reason want to browse a file outside of the diffs... well you go open VIM.
If you "dig in further to get the full file view", you don't see comments. Your proposed addition, as I understand it, is to show comments on the full file view. So it's as far from "how you perform a code review in the GitHub UI" as you can get. There's literally zero overlap.
I'd like to avoid a scenario with Vim is adapted heavily for the purpose (like the diff UI in vim-fugitive).
You're proposing polling and floating windows, you don't think that screams "adapted heavily"?
If code review was happening solely in Vim, I could still use git diff to find what changed,
What does this mean? Call git diff common-ancestor in the console? This is the first and most important step and you're brushing it off like it's a solved problem.
go to that block,
How? I guess you mean jump from :Git diff common-ancestor but that isn't currently supported.
see any comments for that hunk,
Why does this happen now? Why not when viewing the diff? You're so obsessed with the idea of showing comments on the full file that you're blinded to how contrived it is.
Okay let me reconsider design given some of your points. I maybe pushing an idea too quick. Ill take another look and circle back if anything comes of it.