[hacky version] View existing comments in code lines

[wandersoncferreira]

Hello, first of all great package! I came to know this work and I loved it.

I worked in a hacky version to show comments from code lines as described in #5 . Some screenshots from #63 :
and

There are some assumptions here like the ordering of reviews in the github graphql api, all my tests seems to indicate that reviews appear ordered from smaller to higher line numbers.

I am willing to change this code to make it more suitable to the oficial repo. Some guidance is very welcome 👍

Thanks again for your work o/

[wandersoncferreira]

Seems like using body instead of bodyText in the graphql API is possible to identify quotes. Which can be included in the buffer in a different way e.g. ~ > text..

[charignon]

Code looks good, could you place it behind a flag opt-in? This way we can get a few people to test it before it ships by default.

[wandersoncferreira]

Code looks good, could you place it behind a flag opt-in? This way we can get a few people to test it before it ships by default.

I added the flag github-review-view-comments-in-code-lines disabled by default, also included some docs and cleaned up a bit some unnecessary lines.

There a few changes here and there that I don't think needs to be gated e.g. including reviews in graphql query because this is ignored by the rest of the code. But if you feel safer including the new query behind the flag let me know.

[charignon]

Let's put the different query behind a flag too, otherwise the change has a potential performance implication (time to fetch comments) even when you opt out.

[charignon]

Also for the help message let's make explicit what users should set. See the code I removed in pull request 59 for an example.

The rest looks good to me! Almost ready to ship. Thanks for this work.

[wandersoncferreira]

Updated, let me know wdyt.

[charignon]

Please add one test and look into the comment I left, almost good to go! Thank again for your work.

[wandersoncferreira]

Updated, I completely unnoticed the existing tests, sorry. The code now is passing all tests and includes one to validate this feature minimally.

[wandersoncferreira]

[testing, sorry.. wrong pr-id]

[wandersoncferreira]

[testing, sorry.. wrong pr-id]

[charignon]

Are you ready to merge this PR?

[wandersoncferreira]

Are you ready to merge this PR?

Yes, this is ok from my point of view. The comments above were some tests that I used this PR number instead of another one by mistake, disconsider please :)

[charignon]

Looks like the CI is not set properly (my bad) and cannot run the test suite on the PR. As I want to make sure nothing breaks, I will run the tests manually. Alternatively you can send a screenshot of them passing. Then let's merge it!

We should also add a contributor section in the readme if you want to add your name.

[wandersoncferreira]

No worries, tbh while I was working on c9d5382 I noticed some things that can be improved in this PR too.

Do you know exactly how is the relationship between originalPosition, position, and outdated flag?

[wandersoncferreira]

@charignon I think now we are good to go, but let me explain what are the last changes in the codebase:

i) I was under the impression that position field was an ever increasing number after the first @@ mark, but this is true inside a single path. So happen the PRs I was testing had comments in a single file so I didn't notice the issue, but I tested in a complex convoluted PR I had a work and spotted it.

ii) The Github API has a notion for position and originalPosition, a comment might have displacements due to ppl working on the file after the comment was made, in some situations github updates the value for position to reflect the new one, so this new commit is given preference to position instead of originalPosition when available

iii) In some situations a comment can become outdated and its placement in the last version of the diff is hard to do. I added a new flag github-review-view-comments-in-code-lines-outdated if you want to try to place these comments in the buffer, but by default I am filtering out these comments. I don't know if we can have a good UX without adding some sort of "modes" on top of our text file so we could circle back and forth several 'versions' of the PR and place the comments in the right ones. (idk, just an idea that occurred to me).

This is the screenshot of the test suite:

[charignon]

Ok thanks for the explanation and screenshot. When you are good to go I will merge it. Let me know

[wandersoncferreira]

Ok thanks for the explanation and screenshot. When you are good to go I will merge it. Let me know

Guarded behind a flag makes it a lot more comfortable to ship :) thanks for such nice practice. You can merge it.