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

Allowed creation of PR review comments using line number. Fixes #1645 #1649

Closed
wants to merge 7 commits into from
Closed

Allowed creation of PR review comments using line number. Fixes #1645 #1649

wants to merge 7 commits into from

Conversation

AlkahtaniHisham
Copy link

@AlkahtaniHisham AlkahtaniHisham commented Apr 30, 2023

Description

Fixes #1645

Updated org.kohsuke.github.GHPullRequestReviewBuilder to allow creating pull request review comments by line, instead of the deprecated parameter position.

There were some mistakes in my previous PR so I redid it.

Before submitting a PR:

  • Changes must not break binary backwards compatibility. If you are unclear on how to make the change you think is needed while maintaining backward compatibility, CONTRIBUTING.md for details.
  • Add JavaDocs and other comments as appropriate. Consider including links in comments to relevant documentation on https://docs.github.com/en/rest .
  • Add tests that cover any added or changed code. This generally requires capturing snapshot test data. See CONTRIBUTING.md for details.
  • Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.
  • Push your changes to a branch other than main. You will create your PR from that branch.

When creating a PR:

  • Fill in the "Description" above with clear summary of the changes. This includes:
    • If this PR fixes one or more issues, include "Fixes #" lines for each issue.
    • Provide links to relevant documentation on https://docs.github.com/en/rest where possible.
  • All lines of new code should be covered by tests as reported by code coverage. Any lines that are not covered must have PR comments explaining why they cannot be covered. For example, "Reaching this particular exception is hard and is not a particular common scenario."
  • Enable "Allow edits from maintainers".

Copy link

@alexec alexec left a comment

Choose a reason for hiding this comment

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

I think you’ll need to support both line number and position, for backwards compatibility.

They can both be nullable. So change position to be Integer and overload the comment method with a version that takes both position and line.

It would be good to support start_line and side too. This allows for richer multi-line comments.

https://docs.github.com/en/rest/pulls/comments?apiVersion=2022-11-28#create-a-review-comment-for-a-pull-request

@AlkahtaniHisham
Copy link
Author

AlkahtaniHisham commented Apr 30, 2023

github docs say

If you use position, the line, side, start_line, and start_side parameters are not required.

so would it work if I keep the old method (with position) unchanged, and add a method that accepts line, start_line, and side?

@alexec
Copy link

alexec commented Apr 30, 2023

yes, that'd work

Allowed creating PR Review comments by either line or position, added start_line and side options.
Copy link

@alexec alexec left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

Looks reasonable, minor suggestions.
Add tests to exercise the new method.

@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Patch coverage: 29.41% and project coverage change: -0.30 ⚠️

Comparison is base (e45932d) 80.04% compared to head (f15590f) 79.74%.

❗ Current head f15590f differs from pull request most recent head 01e57a0. Consider uploading reports for the commit 01e57a0 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1649      +/-   ##
============================================
- Coverage     80.04%   79.74%   -0.30%     
+ Complexity     2231     2197      -34     
============================================
  Files           215      209       -6     
  Lines          6760     6686      -74     
  Branches        365      364       -1     
============================================
- Hits           5411     5332      -79     
- Misses         1134     1140       +6     
+ Partials        215      214       -1     
Impacted Files Coverage Δ
...org/kohsuke/github/GHPullRequestReviewBuilder.java 52.38% <29.41%> (-20.70%) ⬇️

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link

@alexec alexec left a comment

Choose a reason for hiding this comment

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

Was there some kind of merge conflict with all the .json files?

I never said this, but the use case I have is making multi line suggestions.

@bitwiseman
Copy link
Member

@alexec @AlkahtaniHisham
Could you record new test data?

@bitwiseman bitwiseman self-requested a review June 30, 2023 20:00
@alexec
Copy link

alexec commented Jul 14, 2023

LGTM

@bitwiseman
Copy link
Member

@AlkahtaniHisham We need new test data.

@bitwiseman bitwiseman marked this pull request as draft July 17, 2023 16:39
@gilday
Copy link
Contributor

gilday commented Aug 9, 2023

@bitwiseman I can help record the test data if needed. I understand I would need access to hub4j-test-org to record and check-in interactions with that account.

@bitwiseman
Copy link
Member

@gilday Invitation sent for the test org.

@AlkahtaniHisham AlkahtaniHisham closed this by deleting the head repository Oct 15, 2023
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.

Allow creation of comments using line number, rather than position
4 participants