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

Ignore failed pull request comments #92

Merged
merged 1 commit into from
Sep 20, 2015

Conversation

jhass
Copy link
Contributor

@jhass jhass commented Sep 6, 2015

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 their diff position is
non-existent on Github.

@mmozuras
Copy link
Member

@jhass I assume that GitHub is also using libgit2. We must be able to achieve the same result. Maybe passing the patience option for diff would make it consistent? Wanna try that?

@jhass
Copy link
Contributor Author

jhass commented Sep 20, 2015

Sure, trying to reduce those failures is good, but I view that as a separate change. Maybe there's a logger I can throw a error or fatal at to not completely swallow it? What I want to get rid of is failure of posting further valid comments if there's an invalid one in the middle.

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 their diff position is
non-existent on Github.
@jhass
Copy link
Contributor Author

jhass commented Sep 20, 2015

I added printing a warning to stderr.

mmozuras added a commit that referenced this pull request Sep 20, 2015
Ignore failed pull request comments
@mmozuras mmozuras merged commit 8c68f32 into prontolabs:master Sep 20, 2015
@mmozuras
Copy link
Member

@jhass thanks, you'll be certainly be the MVP of Pronto 0.5.0! 😄

@jhass
Copy link
Contributor Author

jhass commented Sep 20, 2015

Well, I wanted to move away from HoundCI since it ignores our configs all the time, so we tried running Pronto in an automated fashion and found these issues ;)

@jhass
Copy link
Contributor Author

jhass commented Sep 20, 2015

Mmh, looking into preparing to try with the patience diff algo for when I run into a situation where a comment would fail again, rugged mentions the option but says it's not implemented? http:https://www.rubydoc.info/gems/rugged/Rugged%2FTree.diff

@jhass
Copy link
Contributor Author

jhass commented Sep 20, 2015

@mmozuras
Copy link
Member

Well, I wanted to move away from HoundCI since it ignores our configs all the time, so we tried running Pronto in an automated fashion and found these issues ;)

@jhass are those diaspora configs? 😄

Mmh, looking into preparing to try with the patience diff algo for when I run into a situation where a comment would fail again, rugged mentions the option but says it's not implemented?

Seems to be implemented in libgit2, maybe only the docs are out of date: https://github.com/libgit2/libgit2/search?utf8=%E2%9C%93&q=patience

@jhass
Copy link
Contributor Author

jhass commented Sep 20, 2015

are those diaspora configs?

Yeah ;)

@mmozuras
Copy link
Member

@jhass wow. Do tell if you'll end up switching to Pronto 😄

@jhass
Copy link
Contributor Author

jhass commented Sep 20, 2015

Well, Hound is disabled and we're running Pronto (with my patches) for a while now, check the activity of @diaspora-code-review ;)

@jhass jhass deleted the ghpr_ignore_failed branch September 20, 2015 15:46
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