-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add validation to creating review with comments to avoid misleading Github API error message #3294
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
base: main
Are you sure you want to change the base?
Conversation
|
Hey - I'm having trouble locally getting tests to run. Trying to follow the instructions in and trying isn't working either. |
|
Also, if anyone from this project has pro-tips for getting the upstream issue fixed in Github API (probably lolwutno after the layoffs 😢), that feedback is welcome. Looks like they have a mistaken early exit in their review comment validation routine. |
tests/PullRequest.py
Outdated
|
|
||
| def test_validate_review_comment_start_side_start_side_already_defined(self): | ||
| # start_side already defined | ||
| result = self.pull.validate_review_comment_start_side("LEFT", "LEFT", 1, 1, github.GithubObject.NotSet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use a different value for start_side here so we see that start_side does not get modified if given?
| comment.get("start_side", NotSet), | ||
| comment.get("line", NotSet), | ||
| comment.get("start_line", NotSet), | ||
| comment.get("in_reply_to", NotSet), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments when creating a review do not contain the in_reply_to attribute: https://docs.github.com/en/rest/pulls/reviews?apiVersion=2022-11-28#create-a-review-for-a-pull-request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| comment.get("in_reply_to", NotSet), | |
| NotSet, |
Description of issue
The Github API requires
start_sideon multi-line review comments: https://docs.github.com/en/rest/pulls/comments#create-a-review-comment-for-a-pull-requestIf you do not pass this param, and you comment on a multi-line range of deleted code (
LEFT,n,n+1) you get a422validation error with a misleading error message:This misleads the user into thinking the problem is with the
start_lineandlineparams when it is in fact the absentstart_sidethat is the issue.Demonstration of issue:
Script to test issue against this PR
Invoke with:
Proposed fix
Add a validation method on review comment fields to check for the proper combination of fields for multi-line comments.
Optimistically provide
start_sidewhen possible to help things succeed.Assert with a better failure message when not possible to succeed.