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.

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

Show top level comments and reviews #10

Merged
merged 1 commit into from
Mar 10, 2019

Conversation

charignon
Copy link
Owner

@charignon charignon commented Mar 4, 2019

Summary

Before this PR when using github-review one can download PRs title, body and diff and insert top level and inline comments.
After this PR, one can get more information when performing a review, including, reviews from other people, inline comments and issues top-level comments (made by replying to a PR). It also opens future work to support reply to inline comments (#9)

It looks like this:

~ PR TITLE
~
~ PR body
~ on multiple line
~ @foo: a comment at the top level
~ Reviewed by @babar[APPROVED]: LGTM (a review comment)
<actual diff>

Progress

This is really rough work in progress, docstrings aren't final and this isn't fully tested.

  • Top level, issue comments
  • Reviews state and summary
  • Inline comments (moved to another PR)

Test Plan

  • Add testing to start-review for new types of comments
  • Test end to end with multiple scenarios

@charignon
Copy link
Owner Author

charignon commented Mar 9, 2019

Example of what that looks like:
lineendingproblems

I will open another issue for the line ending problem. This also makes me think we should have a mode (derived from diff) for highlighting the usernames and make it more readable.

@charignon charignon force-pushed the lc--add-top-level-comments-by-others branch from 8f46916 to b4592dc Compare March 9, 2019 23:59
@charignon charignon marked this pull request as ready for review March 10, 2019 00:01
@charignon charignon force-pushed the lc--add-top-level-comments-by-others branch from b4592dc to 0b97d91 Compare March 10, 2019 00:06
@charignon
Copy link
Owner Author

I gated this feature behind the flag github-review-fetch-top-level-and-review-comments. I will be testing it for a while and welcome others to do as well. Once I get a better idea of the edge cases / potential issues I will make it the default.

Before this PR when using `github-review` one can download PRs title, body and diff and insert top level and inline comments.
After this PR, one can get more information when performing a review, including, reviews from other people, inline comments and issues top-level comments (made by replying to a PR). It also opens future work to support reply to inline comments (#9)

It looks like this:
```diff
~ PR TITLE
~
~ PR body
~ on multiple line
~ @foo: a comment at the top level
~ Reviewed by @Babar[APPROVED]: LGTM (a review comment)
<actual diff>
```

#### Progress
This is really rough work in progress, docstrings aren't final and this isn't fully tested.
- [X] Top level, issue comments
- [X] Reviews state and summary
- [ ] Inline comments (moved to another Change)

#### Test Plan
- [X] Add testing to start-review for new types of comments
- [X] Test end to end with multiple scenarios

This will enable adding new functionality like showing previous inline
and top level comments.
@charignon charignon force-pushed the lc--add-top-level-comments-by-others branch from 0b97d91 to 446e061 Compare March 10, 2019 00:10
@charignon charignon changed the title [Fix #5][Work In Progress] Show top level comments, reviews, and inline comments [Fix #5][Work In Progress] Show top level comments and reviews Mar 10, 2019
@charignon charignon changed the title [Fix #5][Work In Progress] Show top level comments and reviews Show top level comments and reviews Mar 10, 2019
@charignon charignon merged commit 5b52391 into master Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant