Closed maximevw closed 2 weeks ago
@bitwiseman Would it be possible to give me access to hub4j-test-org in order to finalize this PR by generating test data?
@maximevw
Sending invite now.
Hello @bitwiseman,
I updated the tests to finalize this PR.
I also added these classes to coverage exceptions, as it was already done for org.kohsuke.github.GHPullRequestReviewBuilder.DraftReviewComment
:
org.kohsuke.github.GHPullRequestReviewBuilder.MultilineDraftReviewComment
org.kohsuke.github.GHPullRequestReviewBuilder.SingleLineDraftReviewComment
As it is the same principle, and it does not seem really relevant to write specific tests to cover simple getters in very basic POJOs, I preferred to skip the coverage of these getters.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 80.94%. Comparing base (
e8c6beb
) to head (3df01bb
). Report is 5 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hello @bitwiseman
Regarding the failed coverage test on the changes, as stated in my previous comment, it's only getters and it's very hard to cover these lines. I think we can ignore these warnings as the rest of the changes are covered.
Codecov Report
Attention: Patch coverage is
78.78788%
with7 lines
in your changes missing coverage. Please review.Project coverage is 81.04%. Comparing base (
92112af
) to head (28a18ee
).
I don't know if you still expect anything else from me regarding this PR.
@maximevw
My apologies, but I made a broad change to the test data files resulting in a bunch of conflicts. I've attempted a merge, but I think some of the test files still need updating.
Re: Coverage: I'd prefer to test the getters. They're not hard to do and we've had bugs show up when they weren't. (Side note: we had a glitch in coverage reporting for the past week. I had to fix that before responding to your questions.)
Finally, with the complexity of the API, would it make more sense to have a GHPullRequestCommentBuilder
rather than trying to model the behavior via a bunch of ternary operators in the request call? This feels like it could be a bug farm.
@bitwiseman Thanks for your review, I pushed some changes, as requested:
GHPullRequestReviewCommentBuilder
to avoid too complex operations in the request call method@bitwiseman I pushed the requested modifications and answered to your question regarding the deprecation of GHPullRequestReviewBuilder.comment(String, String, int)
.
Thanks @bitwiseman for the last modifications, I was a little hasty when I pushed my changes... 😕
Description
When creating a new review or adding comments to an existing review, we can currently only add single line comments. This pull request adds support for single and multi-line comments using
line
andstart_line
attributes.See: https://docs.github.com/en/rest/pulls/comments?apiVersion=2022-11-28#create-a-review-comment-for-a-pull-request and https://docs.github.com/en/rest/pulls/reviews?apiVersion=2022-11-28#create-a-review-for-a-pull-request
I already prepared some tests for this and if you provide access to the
hub4j-test-org
, I will be able to finalize them. I also successfully tested these changes on a personal project.This will solve https://github.com/hub4j/github-api/issues/1645
Before submitting a PR:
@link
JavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .mvn -D enable-ci clean install site
locally. If this command doesn't succeed, your change will not pass CI.main
. You will create your PR from that branch.When creating a PR: