Add review comments to PR buffers #105

pwntester · 2021-02-24T23:33:27Z

pwntester · 2021-02-26T23:00:05Z

@tami5 @weilbith the core functionality should be already working. I changed quite a few things so may have broke something elsewhere, still need to do more testing.

The UI stil need improvements though, any suggestions are welcome.

weilbith

This is kind of a test review for this PR features. But I have the following issues which might not be related directly to it. When I now call Octo pr list I get the following error I did not before (but could also be a Telescope update:

Error executing vim.schedule lua callback: ...test_site/pack/packer/opt/octo.nvim/lua/octo/writers.lua:129: bad argument #1 to 'ipairs' (table expected, got nil)

EDIT:
The other issues is that if I hit <CR> while writing the final submit message I get an error (I can't see atm). And the second time I hit the key it just submitted it...

weilbith · 2021-02-27T23:28:15Z

While I was hitting <space>ca to add this comment (for open an actual review conversation thread), I got this error:

E5108: Error executing lua ...test_site/pack/packer/opt/octo.nvim/lua/octo/reviews.lua:268: attempt to call field 'write_virtual_texxt' (a nil value)

And while I tried to paste this error message with "+p I got the error: clipboard error: -2. I pasted it now in insert mode with <C-S-v> by my terminal emulator.

And again while I try to safe this buffer to add a review comment I get the error: E382: Cannot write, 'buftype' open is set. The command echo &buftype returns nofile.

Obviously I was not able to add this comment via Octo. So I copied it and pasted it to the web-browser.

pwntester · 2021-02-27T23:37:45Z

This is kind of a test review for this PR features. But I have the following issues which might not be related directly to it. When I now call Octo pr list I get the following error I did not before (but could also be a Telescope update:

Just fixed it

The other issues is that if I hit while writing the final submit message I get an error (I can't see atm). And the second time I hit the key it just submitted it...

Are you using delimate or similar plugin?

/packer/opt/octo.nvim/lua/octo/reviews.lua:268: attempt to call field 'write_virtual_texxt' (a nil value)

Ups, that was a typo in a recent refactor, should be fixed now

And while I tried to paste this error message with "+p I got the error: clipboard error: -2. I pasted it now in insert mode with by my terminal emulator.

Not sure about this one, not sure is Octo related

And again while I try to safe this buffer to add a review comment I get the error: E382: Cannot write, 'buftype' open is set. The command echo &buftype returns nofile.

This could be caused by any of the previous errors. Please try again with latest fixes

Thanks!

weilbith · 2021-02-27T23:55:10Z

Thanks for the fast response! 🙃

Are you using delimate or similar plugin?

Yes. In combination with nvim-compe (which is wrapping the <CR> mapping)

I'll check it again tomorrow. Or more precisely at daytime of today (for my timezone). ^^

Btw: would it be possible to set the filetype for the buffer to add a comment to markdown? Would be quite helpful.

pwntester · 2021-02-28T15:24:51Z

Btw: would it be possible to set the filetype for the buffer to add a comment to markdown? Would be quite helpful.

Sure, I implemented it

Yes. In combination with nvim-compe (which is wrapping the mapping)

I was able to reproduce the problem with the same combo. Using just compe w/o delimate does not create any issues though so the problem seems to be caused by delimate. Will take a look

EDIT: for now, I remapped in insert mode to to override any other mappings. This solves the problem but leaves you with no completion

weilbith

Just some text for test purpose.

README.md

weilbith · 2021-02-28T22:12:21Z

The indentation of the actual comment text in the thread is off because of these surrounding lines on the left site. If this gets correctly aligned it would look better.

weilbith · 2021-02-28T22:15:42Z

Could you add some context lines around the commented line in a review thread? Maybe a configurable number of how many lines above and below the hunk (if multiple lines got selected).
Later in the pop-issue it would be great to see the code snippet in actual highlighting and other features of the actual file buffer (e.g. [type] hints, signatures, references, etc. pp).

weilbith · 2021-02-28T22:19:55Z

And if I could color the text like THREAD, the date etc would be great. Atm it is too greyish white for me. I need to improve my markdown concealing and stuff, but its a bit hard to strongly differ between "UI" and user written content. Maybe event the lines that surround comments. Doesn't hurt to open the highlight groups if not already. Sorry if I just missed this group. 🙈

lua/octo/reviews.lua

weilbith · 2021-02-28T22:27:41Z

@pwntester sorry that I have so many comments. I love to play around with this feature. It is so great! 😊 🚀
This feature is so big, you might want to merge it and open issues/PRs as follow ups to tackle certain problems. Else this might take very long. 🙈

lua/octo/util.lua

pwntester · 2021-02-28T22:41:31Z

The indentation of the actual comment text in the thread is off because of these surrounding lines on the left site. If this gets correctly aligned it would look better.

Not sure I understood what you mean. Those surrounding lines are the sign column, cannot change indentation.

pwntester · 2021-02-28T22:44:02Z

Could you add some context lines around the commented line in a review thread? Maybe a configurable number of how many lines above and below the hunk (if multiple lines got selected).

This is what the GitHub UI shows currently. I'm getting it from the diffhunk, so it would be limited to lines appearing in the hunk, otherwise I would need to make an additional call to the API to get the file contents.

pwntester · 2021-02-28T22:56:20Z

And if I could color the text like THREAD, the date etc would be great. Atm it is too greyish white for me. I need to improve my markdown concealing and stuff, but its a bit hard to strongly differ between "UI" and user written content. Maybe event the lines that surround comments. Doesn't hurt to open the highlight groups if not already. Sorry if I just missed this group. 🙈

That should be OctoNvimMissingDetails but I think I should make a new HL for it because that one was meant for a different thing and I just reused it so I just created OctoNvimTimelineItemHeading

Any ideas about how to differentiate the virtual text lines from the editable blocks?
Do you like the use of uppercase for THREAD, REVIEW, COMMENT, etc?

pwntester · 2021-02-28T23:03:44Z

@pwntester sorry that I have so many comments. I love to play around with this feature. It is so great! 😊 🚀
This feature is so big, you might want to merge it and open issues/PRs as follow ups to tackle certain problems. Else this might take very long. 🙈

Yep, I will use this branch this week at work to catch up more bugs and then merge it. I think its functional, just the UI is not as optimal as it should be. But its hard to get what I would like with just a text buffer. If only I could soft indent lines of the buffer 🙄

weilbith · 2021-02-28T23:09:50Z

That should be OctoNvimMissingDetails but I think I should make a new HL for it because that one was meant for a different thing and I just reused it so I just created OctoNvimTimelineItemHeading

Ah nice. I'll use it. Let's see how I can beautify this for my setup.

Any ideas about how to differentiate the virtual text lines from the editable blocks?

Hmm. Good question. The only dumb thing that comes into my mind would be a highlighting that only affects the background color, which then subtle differs from the background. But despite aesthetic issues this suffers from the same issue as the treesitter-refactor context highlight as you could only highlight he background of the actual text, not a box around the text including "imaginary" whitespaces behind shorter lines.

Do you like the use of uppercase for THREAD, REVIEW, COMMENT, etc?

Yes I do. Perfectly nice from point of view. 👌

weilbith · 2021-02-28T23:12:01Z

This is what the GitHub UI shows currently. I'm getting it from the diffhunk, so it would be limited to lines appearing in the hunk, otherwise I would need to make an additional call to the API to get the file contents.

Oh really. Hmm. If kinda differs in the web UI for me. But that's fine I guess and could be improved later.
The only other difference would be that GH UI does show old and new context compared to each other what is missing here. So as you don't have the split diff view here, you have a hard time to tell what the actual change was that took place here. 🤷

weilbith · 2021-02-28T23:13:25Z

Not sure I understood what you mean. Those surrounding lines are the sign column, cannot change indentation.

Oh. Wait. I need to investigate there. Maybe the planned folding would improve this. 🤔
I need to check some things and setting options to "see" it better. But ofc it makes sense that this is not part of the actual buffer content. :face_palm:

pwntester · 2021-03-01T09:35:54Z

Recap:

Bugs:

when adding a new review comment the cursor goes to the QF window
off-by-one box-line issue on empty lines of commented lines hunks
horizontal split when commenting on the LEFT side?
escape double quotes on gh layer

Questions:

Also the content from the cite above was in the comment buffer initially.

What do you mean here? do you refer to the path [RIGHT|LEFT] (start, end) virtual text? should I remove it?

Hmm. Good question. The only dumb thing that comes into my mind would be a highlighting that only affects the background color, which then subtle differs from the background. But despite aesthetic issues this suffers from the same issue as the treesitter-refactor context highlight as you could only highlight he background of the actual text, not a box around the text including imaginary whitespaces behind shorter lines.

Just implemented this. It will apply OctoNvimEditable which defaults to a highlight with same background color as NormalFloat
I would prefer to soft wrap at the end to avoid hl to be applied to the whole line. Maybe we can do something like Goyo does and add a small padding vert split to the right

weilbith · 2021-03-01T10:51:19Z

when adding a new review comment the cursor goes to the QF window
horizontal split when commenting on the LEFT side?

I think those are related as I have described here

when adding a suggestion use a filetype rather than suggestion so at editing time highlght is applied. then it can be swaped with suggestion before commiting so GH treats it as a suggestion

This PR is really packed so I already exported this into a new issue #108 You do so much in this PR. It should come to an end and cut of new features where possible. Sorry for all this stress. 🙈

add more context around the commented lines hunk and show both left/right versions for the commented lines range

Again I would split this into a new issue. The PR solution is totally fine for the moment. It can be improved later. This is not straight forward as the solution approach/goal is not clear. There are multiple opportunities for this issue. Do you agree? I would open the issue then. So it can be discussed there and tackled when ever someone has time and motiviation.

on PR/Thread buffers suggestions should show the diff and not just the new suggestion

Same as before. I think the PR solution is fine for the moment and it could be a new issue to improve.

What do you mean here? do you refer to the path [RIGHT|LEFT] (start, end) virtual text? should I remove it?

Yeah, sorry the cite was not visible because it was actual a virtual text where I put a real > cite initiator before. I left a comment here regarding this virtual text. Atm it is very confusing. I don't know your intention behind it. I the user forgot which line he wanted to comment he can cancel and start again. Or maybe provide a hint in the file name the user sees in his status line?

Just implemented this. It will apply OctoNvimEditable which defaults to a highlight with same background color as NormalFloat
I would prefer to soft wrap at the end to avoid hl to be applied to the whole line. Maybe we can do something like Goyo does and add a small padding vert split to the right

Amazing! 🤩
I have an open issue at the tressitter plugin. There they say it is not possible. How did you solve it. Could you leave a comment at nvim-treesitter/nvim-treesitter-refactor#12 ? But maybe your approach is not usable for "real" file buffer of source code. 🤷

I think the open bugs are legit to get solved in this PR. But all further improvements should be exported. This PR is already huge and you introduced a complete new big feature "section" in your plugin. Also you did already add tons of nice additions as reaction to my comments. 🙏

pwntester · 2021-03-01T11:06:59Z

Yeah, sorry the cite was not visible because it was actual a virtual text where I put a real > cite initiator before. I left a comment here regarding this virtual text. Atm it is very confusing. I don't know your intention behind it. I the user forgot which line he wanted to comment he can cancel and start again. Or maybe provide a hint in the file name the user sees in his status line?

Agreed, I will remove that virtual text

I have an open issue at the tressitter plugin. There they say it is not possible. How did you solve it. Could you leave a comment at nvim-treesitter/nvim-treesitter-refactor#12 ? But maybe your approach is not usable for "real" file buffer of source code. 🤷

will do

I think the open bugs are legit to get solved in this PR. But all further improvements should be exported. This PR is already huge and you introduced a complete new big feature "section" in your plugin. Also you did already add tons of nice additions as reaction to my comments. 🙏

agreed, will try to fix the bugs, and all suggestions can be moved into new issues

Thanks!
asdfasdf #94 asdasdf

weilbith · 2021-03-01T11:32:25Z

Alright. I opened a bunch of new issues.
You can remove (or strike out) them from your feature todo list.
Also you can close all review conversations in this PR (as I can't).

This plugin is so damn amazing. Dreamed of it for a long time. 😍

weilbith · 2021-03-01T19:03:20Z

Could you make the highlighting of the editable stuff have a lower priority? For me it does overwrite the highlighting of the Issue title, id and status. If I set only the background color for the OctoNvimEditable group it works fine. But for better reusability of my colorscheme, it would great to link it to a group that has back- and foreground color, but the other highlight groups like OctoNvimIssueTitle can still draw over it. Would be great!

pwntester

final

pwntester · 2021-03-03T16:58:12Z

README.md

+| thread | resolve| Mark a review thread as resolved |
+| | unresolve | Mark a review thread as unresolved |


pwntester added 2 commits February 25, 2021 00:25

add review comments to pull request query

ffb5c1b

initial implementation of review comments in PR buffer

806c032

pwntester mentioned this pull request Feb 24, 2021

Feature request - have PR review converstations in the PR buffer #100

Closed

pwntester added 4 commits February 25, 2021 11:35

better sorting/grouping of review/reviewthreads

f3cc55e

multifold suppport + thread header stub

d686206

prettified diff hunk visualization on review threads

1bdf9bd

pretify thread header

c2a7240

pwntester added 4 commits February 27, 2021 00:18

update README

6097106

add some test conceals for GH emojis

abbd01f

Add initial working impl for review/threads in PR buffers

b7cca29

use v3 REST API to post thread replies

75fe99f

pwntester marked this pull request as ready for review February 27, 2021 12:07

pwntester self-assigned this Feb 27, 2021

pwntester added 2 commits February 27, 2021 23:28

refactor calls to nvim_buf_set_virtual_text

4fea563

use timelineitems to fetch sorted timeline events

06622ff

weilbith reviewed Feb 27, 2021

View reviewed changes

pwntester added 2 commits February 28, 2021 00:31

fomatting

684de67

fix error in Pull request telescope preview

c424b8d

pwntester added 2 commits February 28, 2021 00:37

fix typo

347d022

prevent NPE

3680100

markdown everywhere!

7f34456

prevent delimimate crash on review submit buffer

a7acd2a

weilbith reviewed Feb 28, 2021

View reviewed changes

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

weilbith reviewed Feb 28, 2021

View reviewed changes

lua/octo/reviews.lua Outdated Show resolved Hide resolved

weilbith reviewed Feb 28, 2021

View reviewed changes

lua/octo/util.lua Show resolved Hide resolved

add new HL groups

172f339

some work on signs

b1cfbc4

Use a different background for editable blocks

34fefa1

pwntester mentioned this pull request Mar 1, 2021

Highlight current scope - full buffer width? nvim-treesitter/nvim-treesitter-refactor#12

Open

pwntester added 3 commits March 1, 2021 12:17

add new emoji support batch

607c5e7

remove comment buffer header

b6743b0

escape double quotes on graphql api parameters

4b7258a

allow unescaped queries

348bb3b

pwntester added 2 commits March 1, 2021 23:56

use float window for adding comments

597e2f8

do not set comment buffer as not modified

647bb62

pwntester merged commit 2bf0f47 into master Mar 1, 2021

pwntester deleted the add_reviews_to_prs branch March 1, 2021 23:06

pwntester commented Mar 3, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add review comments to PR buffers #105

Add review comments to PR buffers #105

pwntester commented Feb 24, 2021 •

edited

Loading

pwntester commented Feb 26, 2021

weilbith left a comment •

edited

Loading

weilbith commented Feb 27, 2021 •

edited

Loading

pwntester commented Feb 27, 2021

weilbith commented Feb 27, 2021 •

edited

Loading

pwntester commented Feb 28, 2021 •

edited

Loading

weilbith left a comment

weilbith commented Feb 28, 2021 •

edited by pwntester

Loading

weilbith commented Feb 28, 2021

weilbith commented Feb 28, 2021

weilbith commented Feb 28, 2021

pwntester commented Feb 28, 2021

pwntester commented Feb 28, 2021

pwntester commented Feb 28, 2021

pwntester commented Feb 28, 2021

weilbith commented Feb 28, 2021

weilbith commented Feb 28, 2021

weilbith commented Feb 28, 2021 •

edited

Loading

pwntester commented Mar 1, 2021 •

edited

Loading

weilbith commented Mar 1, 2021 •

edited

Loading

pwntester commented Mar 1, 2021 •

edited

Loading

weilbith commented Mar 1, 2021

weilbith commented Mar 1, 2021 •

edited

Loading

pwntester left a comment

pwntester Mar 3, 2021

		\| thread \| resolve\| Mark a review thread as resolved \|
		\| \| unresolve \| Mark a review thread as unresolved \|

Add review comments to PR buffers #105

Add review comments to PR buffers #105

Conversation

pwntester commented Feb 24, 2021 • edited Loading

pwntester commented Feb 26, 2021

weilbith left a comment • edited Loading

Choose a reason for hiding this comment

weilbith commented Feb 27, 2021 • edited Loading

pwntester commented Feb 27, 2021

weilbith commented Feb 27, 2021 • edited Loading

pwntester commented Feb 28, 2021 • edited Loading

weilbith left a comment

Choose a reason for hiding this comment

weilbith commented Feb 28, 2021 • edited by pwntester Loading

weilbith commented Feb 28, 2021

weilbith commented Feb 28, 2021

weilbith commented Feb 28, 2021

pwntester commented Feb 28, 2021

pwntester commented Feb 28, 2021

pwntester commented Feb 28, 2021

pwntester commented Feb 28, 2021

weilbith commented Feb 28, 2021

weilbith commented Feb 28, 2021

weilbith commented Feb 28, 2021 • edited Loading

pwntester commented Mar 1, 2021 • edited Loading

weilbith commented Mar 1, 2021 • edited Loading

pwntester commented Mar 1, 2021 • edited Loading

weilbith commented Mar 1, 2021

weilbith commented Mar 1, 2021 • edited Loading

pwntester left a comment

Choose a reason for hiding this comment

pwntester Mar 3, 2021

Choose a reason for hiding this comment

pwntester commented Feb 24, 2021 •

edited

Loading

weilbith left a comment •

edited

Loading

weilbith commented Feb 27, 2021 •

edited

Loading

weilbith commented Feb 27, 2021 •

edited

Loading

pwntester commented Feb 28, 2021 •

edited

Loading

weilbith commented Feb 28, 2021 •

edited by pwntester

Loading

weilbith commented Feb 28, 2021 •

edited

Loading

pwntester commented Mar 1, 2021 •

edited

Loading

weilbith commented Mar 1, 2021 •

edited

Loading

pwntester commented Mar 1, 2021 •

edited

Loading

weilbith commented Mar 1, 2021 •

edited

Loading