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

feat(framework-core): support inline multiline comments suggestions on pull requests #105

Merged
merged 15 commits into from
Sep 25, 2020

Conversation

TomKristie
Copy link
Contributor

Framework-core logic for creating a multi-line inline comments suggesting code to a pull request given the pull request number, and the old and new contents of files that belong to the pull request.

Specifically given the input of the PR number, and a subset of the PRs files old and new file contents:

Generate the diff range between the old and new contents for each file provided by the user. This is each potential file's potential multiline comments domain. And then from the potential suggestions, create in-scope suggestions by:
Filtering out-of-scope suggestions by:
a. Filter out the files who are not in the PR (creating in-scope files)
b. Filter out the non-filtered file's lines that are out of scope of the PR (creating in-scope files with in-scope lines), and
c. Filter out the files who were too large to load the diff since the file is too large to comment on (this is tentative to change)
Take the in-scope suggestion ranges for each file and get the corresponding text.
on the PR number, make the in-scope suggestions.
Comment on what could not be commented on and why.
The remaining result is a diff range for files and file lines that are in-scope of the pull request.

Framework-core
Towards #59

TomKristie and others added 7 commits August 17, 2020 11:05
* fix(patch text to hunk bounds): support regex for patch texts

* more comments and more tests
… the valid hunks (#86)

* fix(framework-core): parse raw changes to ranges
* test: add failing stub and test for building the failure message

* fix: implement message building

* fix: use original line numbers in error message

* docs: add docstring

* docs: add note about empty input returning empty string
@TomKristie TomKristie requested a review from a team September 8, 2020 04:35
@TomKristie TomKristie requested a review from a team as a code owner September 8, 2020 04:35
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 8, 2020
@codecov
Copy link

codecov bot commented Sep 8, 2020

Codecov Report

Merging #105 into master will increase coverage by 6.61%.
The diff coverage is 99.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
+ Coverage   85.28%   91.89%   +6.61%     
==========================================
  Files          14       25      +11     
  Lines        1257     2295    +1038     
  Branches       87      182      +95     
==========================================
+ Hits         1072     2109    +1037     
- Misses        184      185       +1     
  Partials        1        1              
Impacted Files Coverage Δ
src/github-handler/index.ts 94.73% <93.33%> (+19.73%) ⬆️
...ler/make-review-handler/upload-comments-handler.ts 99.20% <99.20%> (ø)
src/default-options-handler.ts 100.00% <100.00%> (ø)
...et-hunk-scope-handler/github-patch-text-handler.ts 100.00% <100.00%> (ø)
...er/comment-handler/get-hunk-scope-handler/index.ts 100.00% <100.00%> (ø)
...-hunk-scope-handler/remote-patch-ranges-handler.ts 100.00% <100.00%> (ø)
src/github-handler/comment-handler/index.ts 100.00% <100.00%> (ø)
...ndler/comment-handler/make-review-handler/index.ts 100.00% <100.00%> (ø)
...ent-handler/make-review-handler/message-handler.ts 100.00% <100.00%> (ø)
...handler/raw-patch-handler/hunk-to-patch-handler.ts 100.00% <100.00%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c61da3e...1795930. Read the comment docs.

Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're missing some test coverage on the main comment/reviewPullRequest logic.

src/github-handler/comment-handler/index.ts Outdated Show resolved Hide resolved
src/github-handler/comment-handler/index.ts Outdated Show resolved Hide resolved
TomKristie and others added 2 commits September 10, 2020 22:00
…est (#114)

* feat(framework-core): main interface for create review on a pull request

* docs: fix typo

* nits and typos...

* gts lint warning fix
@chingor13 chingor13 added the cla: yes This human has signed the Contributor License Agreement. label Sep 11, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 11, 2020
* fix(framework-core): collapsing timeline and inline comments into single review

* test: fixed imports

* added case when there are out of scope suggestions and no valid suggestions
Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really close!

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
repo: remote.repo,
pull_number: pullNumber,
commit_id: headSha,
event: 'COMMENT',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feature request: pipe an option to make this 'REQUEST_CHANGES' or 'COMMENT'. This way someone could configure it to be a blocking review.

@bcoe bcoe added the cla: yes This human has signed the Contributor License Agreement. label Sep 24, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 24, 2020
* feat(framework-core): return review number and variable renaming

* lint
@chingor13 chingor13 added the cla: yes This human has signed the Contributor License Agreement. label Sep 25, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 25, 2020
@TomKristie
Copy link
Contributor Author

TomKristie commented Sep 25, 2020

Wow very exciting! I don't have permissions to merge so that is up to you @chingor13 or @bcoe . Thanks for your help all along this pr. 😄

@chingor13 chingor13 merged commit 415fb8a into master Sep 25, 2020
@chingor13 chingor13 deleted the comment-pr branch September 25, 2020 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants