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

Improve GithubPullRequestFormatter and related #90

Closed
wants to merge 4 commits into from

Conversation

jhass
Copy link
Contributor

@jhass jhass commented Sep 5, 2015

Please see the commit messages.

https://developer.github.com/v3/pulls/comments/#create-a-comment
appears to be misleading or wrong, depending on how you interpret it.

commit_id is meant to reference the "state" of the PR when creating the
commit, that is it references the most recent commit of the PR, always.

position is meant to be the position in the full diff, not the position
in any single commit's diff.

With full diff the combined diff of all commits is meant, as you get for
example via /:owner/:repo/pull/:id.diff
By default Octokit only fetches the first 30 comments of a pull request,
on large pull requests that makes the duplicate comment detection fail,
leading to duplicates.
The diff output of the local git version and Github is not always consistent
especially in areas where file renames happened,  Github tends to recognize
these better, leading to messages we can't post because they diff position is
non-existent on Github.
@mmozuras
Copy link
Member

mmozuras commented Sep 6, 2015

@jhass could you split it up into four PRs?

@jhass
Copy link
Contributor Author

jhass commented Sep 6, 2015

I could but given I needed all of these after running pronto in an automated fashion for a while, I'd need all of them in or fork.

@jhass
Copy link
Contributor Author

jhass commented Sep 6, 2015

Okay anyway, superseded by #91 #92 #93 #94

@jhass jhass closed this Sep 6, 2015
@jhass jhass deleted the pr_review_fixes branch September 7, 2015 16:56
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.

None yet

2 participants